Performance enhancements for Cleanup Tracks & Vias.Performance enhancements for Cleanup Tracks & Vias.Performance enhancements for Cleanup Tracks & Vias.Performance enhancements for Cleanup Tracks & Vias.Performance enhancements for Cleanup Tracks & Vias.Performance enhancements for Cleanup Tracks & Vias.Performance enhancements for Cleanup Tracks & Vias.Performance enhancements for Cleanup Tracks & Vias.Performance enhancements for Cleanup Tracks & Vias.

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

pcbnew/cleanup_item.cpp

@@ -43,21 +43,20 @@ wxString CLEANUP_ITEM::GetErrorText( int aCode, bool aTranslate ) const
     switch( aCode )
     {
     // For cleanup tracks and vias:
-    case CLEANUP_CHECKING_ZONE_FILLS:   msg = _HKI( "Checking zone fills..." );             break;
+    case CLEANUP_SHORTING_TRACK:    msg = _HKI( "Remove track shorting two nets" );          break;
-    case CLEANUP_SHORTING_TRACK:        msg = _HKI( "Remove track shorting two nets" );     break;
+    case CLEANUP_SHORTING_VIA:      msg = _HKI( "Remove via shorting two nets" );            break;
-    case CLEANUP_SHORTING_VIA:          msg = _HKI( "Remove via shorting two nets" );       break;
+    case CLEANUP_REDUNDANT_VIA:     msg = _HKI( "Remove redundant via" );                    break;
-    case CLEANUP_REDUNDANT_VIA:         msg = _HKI( "Remove redundant via" );               break;
+    case CLEANUP_DUPLICATE_TRACK:   msg = _HKI( "Remove duplicate track" );                  break;
-    case CLEANUP_DUPLICATE_TRACK:       msg = _HKI( "Remove duplicate track" );             break;
+    case CLEANUP_MERGE_TRACKS:      msg = _HKI( "Merge co-linear tracks" );                  break;
-    case CLEANUP_MERGE_TRACKS:          msg = _HKI( "Merge co-linear tracks" );             break;
+    case CLEANUP_DANGLING_TRACK:    msg = _HKI( "Remove track not connected at both ends" ); break;
-    case CLEANUP_DANGLING_TRACK:        msg = _HKI( "Remove track not connected at both ends" );       break;
+    case CLEANUP_DANGLING_VIA:      msg = _HKI( "Remove via connected on fewer than two layers" ); break;
-    case CLEANUP_DANGLING_VIA:          msg = _HKI( "Remove via connected on fewer than two layers" ); break;
+    case CLEANUP_ZERO_LENGTH_TRACK: msg = _HKI( "Remove zero-length track" );                break;
-    case CLEANUP_ZERO_LENGTH_TRACK:     msg = _HKI( "Remove zero-length track" );           break;
+    case CLEANUP_TRACK_IN_PAD:      msg = _HKI( "Remove track inside pad" );                 break;
-    case CLEANUP_TRACK_IN_PAD:          msg = _HKI( "Remove track inside pad" );            break;
 
     // For cleanup graphics:
-    case CLEANUP_NULL_GRAPHIC:          msg = _HKI( "Remove zero-size graphic" );           break;
+    case CLEANUP_NULL_GRAPHIC:      msg = _HKI( "Remove zero-size graphic" );                break;
-    case CLEANUP_DUPLICATE_GRAPHIC:     msg = _HKI( "Remove duplicated graphic" );          break;
+    case CLEANUP_DUPLICATE_GRAPHIC: msg = _HKI( "Remove duplicated graphic" );               break;
-    case CLEANUP_LINES_TO_RECT:         msg = _HKI( "Convert lines to rectangle" );         break;
+    case CLEANUP_LINES_TO_RECT:     msg = _HKI( "Convert lines to rectangle" );              break;
 
     default:
         wxFAIL_MSG( wxT( "Missing cleanup item description" ) );

pcbnew/cleanup_item.h

@@ -31,8 +31,7 @@ class PCB_BASE_FRAME;
 
 enum CLEANUP_RC_CODE {
     CLEANUP_FIRST = DRCE_LAST + 1,
-    CLEANUP_CHECKING_ZONE_FILLS = CLEANUP_FIRST,
+    CLEANUP_SHORTING_TRACK = CLEANUP_FIRST,
-    CLEANUP_SHORTING_TRACK,
     CLEANUP_SHORTING_VIA,
     CLEANUP_REDUNDANT_VIA,
     CLEANUP_DUPLICATE_TRACK,

pcbnew/dialogs/dialog_cleanup_tracks_and_vias.cpp

@@ -131,24 +131,20 @@ void DIALOG_CLEANUP_TRACKS_AND_VIAS::doCleanup( bool aDryRun )
 
     if( m_firstRun )
     {
-        m_items.push_back( std::make_shared<CLEANUP_ITEM>( CLEANUP_CHECKING_ZONE_FILLS ) );
-        RC_ITEMS_PROVIDER* provider = new VECTOR_CLEANUP_ITEMS_PROVIDER( &m_items );
-        m_changesTreeModel->SetProvider( provider );
-
         m_reporter->Report( _( "Check zones..." ) );
+        wxSafeYield();      // Timeslice to update UI
         m_parentFrame->GetToolManager()->GetTool<ZONE_FILLER_TOOL>()->CheckAllZones( this );
         wxSafeYield();      // Timeslice to close zone progress reporter
-
-        m_changesTreeModel->SetProvider( nullptr );
-        m_items.clear();
         m_firstRun = false;
     }
 
     // Old model has to be refreshed, GAL normally does not keep updating it
     m_reporter->Report( _( "Rebuild connectivity..." ) );
+    wxSafeYield();      // Timeslice to update UI
     m_parentFrame->Compile_Ratsnest( false );
 
     m_reporter->Report( _( "Check items..." ) );
+    wxSafeYield();      // Timeslice to update UI
 
     cleaner.CleanupBoard( aDryRun, &m_items, m_cleanShortCircuitOpt->GetValue(),
                                              m_cleanViasOpt->GetValue(),
@@ -159,6 +155,7 @@ void DIALOG_CLEANUP_TRACKS_AND_VIAS::doCleanup( bool aDryRun )
                                              m_reporter );
 
     m_reporter->Report( _( "Items checked..." ) );
+    wxSafeYield();      // Timeslice to update UI
 
     if( aDryRun )
     {

pcbnew/tracks_cleaner.cpp

@@ -62,20 +62,29 @@ void TRACKS_CLEANER::CleanupBoard( bool aDryRun, std::vector<std::shared_ptr<CLE
     m_itemsList = aItemsList;
 
     if( m_reporter )
+    {
         m_reporter->Report( _( "Clean vias and tracks" ) );
+        wxSafeYield();      // Timeslice to update UI
+    }
 
     bool removeNullSegments = aMergeSegments || aRemoveMisConnected;
     cleanup( aCleanVias, removeNullSegments, aMergeSegments /* dup segments*/, aMergeSegments );
 
     if( m_reporter )
+    {
         m_reporter->Report( _( "Merge collinear tracks" ) );
+        wxSafeYield();      // Timeslice to update UI
+    }
 
      cleanup( false, false, true, aMergeSegments );
 
     if( aRemoveMisConnected )
     {
         if( m_reporter )
+        {
             m_reporter->Report( _( "Remove misconnected" ) );
+            wxSafeYield();      // Timeslice to update UI
+        }
 
         removeShortingTrackSegments();
     }
@@ -83,7 +92,10 @@ void TRACKS_CLEANER::CleanupBoard( bool aDryRun, std::vector<std::shared_ptr<CLE
     if( aDeleteTracksinPad )
     {
         if( m_reporter )
+        {
             m_reporter->Report( _( "Delete tracks in pads" ) );
+            wxSafeYield();      // Timeslice to update UI
+        }
 
         deleteTracksInPads();
     }
@@ -93,7 +105,10 @@ void TRACKS_CLEANER::CleanupBoard( bool aDryRun, std::vector<std::shared_ptr<CLE
     if( has_deleted && aMergeSegments )
     {
         if( m_reporter )
+        {
             m_reporter->Report( _( "Merge segments" ) );
+            wxSafeYield();      // Timeslice to update UI
+        }
 
         cleanup( false, false, false, true );
     }
@@ -428,20 +443,25 @@ void TRACKS_CLEANER::cleanup( bool aDeleteDuplicateVias, bool aDeleteNullSegment
         {
             merged = false;
             m_brd->BuildConnectivity();
+
             auto connectivity = m_brd->GetConnectivity()->GetConnectivityAlgo();
 
             // Keep a duplicate deque to all deleting in the primary
             std::deque<PCB_TRACK*> temp_segments( m_brd->Tracks() );
 
+            m_connectedItemsCache.clear();
+
             // merge collinear segments:
             for( PCB_TRACK* segment : temp_segments )
             {
-                if( segment->Type() != PCB_TRACE_T )    // one can merge only track collinear segments, not vias.
+                // one can merge only collinear segments, not vias or arcs.
+                if( segment->Type() != PCB_TRACE_T )
                     continue;
 
-                if( segment->HasFlag( IS_DELETED ) )  // already taken in account
+                if( segment->HasFlag( IS_DELETED ) )  // already taken into account
                     continue;
 
+                // for each end of the segment:
                 for( CN_ITEM* citem : connectivity->ItemEntry( segment ).GetItems() )
                 {
                     // Do not merge an end which has different width tracks attached -- it's a
@@ -454,25 +474,34 @@ void TRACKS_CLEANER::cleanup( bool aDeleteDuplicateVias, bool aDeleteNullSegment
                         if( !connected->Valid() )
                             continue;
 
-                        BOARD_CONNECTED_ITEM* candidateItem = connected->Parent();
+                        BOARD_CONNECTED_ITEM* candidate = connected->Parent();
 
-                        if( candidateItem->Type() == PCB_TRACE_T && !candidateItem->HasFlag( IS_DELETED ) )
+                        if( candidate->Type() == PCB_TRACE_T && !candidate->HasFlag( IS_DELETED ) )
                         {
-                            PCB_TRACK* candidateSegment = static_cast<PCB_TRACK*>( candidateItem );
+                            PCB_TRACK* candidateSegment = static_cast<PCB_TRACK*>( candidate );
 
                             if( candidateSegment->GetWidth() == segment->GetWidth() )
+                            {
                                 sameWidthCandidates.push_back( candidateSegment );
+                            }
                             else
+                            {
                                 differentWidthCandidates.push_back( candidateSegment );
+                                break;
+                            }
                         }
                     }
 
-                    if( differentWidthCandidates.size() == 0 )
+                    if( !differentWidthCandidates.empty() )
+                        continue;
+
+                    for( PCB_TRACK* candidate : sameWidthCandidates )
                     {
-                        for( PCB_TRACK* candidate : sameWidthCandidates )
+                        if( segment->ApproxCollinear( *candidate )
+                                && mergeCollinearSegments( segment, candidate ) )
                         {
-                            if( segment->ApproxCollinear( *candidate ) )
+                            merged = true;
-                                merged |= mergeCollinearSegments( segment, candidate );
+                            break;
                         }
                     }
                 }
@@ -485,64 +514,76 @@ void TRACKS_CLEANER::cleanup( bool aDeleteDuplicateVias, bool aDeleteNullSegment
 }
 
 
+const std::vector<BOARD_CONNECTED_ITEM*>& TRACKS_CLEANER::getConnectedItems( PCB_TRACK* aTrack )
+{
+    const std::shared_ptr<CONNECTIVITY_DATA>& connectivity = m_brd->GetConnectivity();
+
+    if( m_connectedItemsCache.count( aTrack ) == 0 )
+    {
+        m_connectedItemsCache[ aTrack ] =
+                connectivity->GetConnectedItems( aTrack, { PCB_TRACE_T, PCB_ARC_T, PCB_VIA_T,
+                                                           PCB_PAD_T, PCB_ZONE_T } );
+    }
+
+    return m_connectedItemsCache[ aTrack ];
+}
+
+
 bool TRACKS_CLEANER::mergeCollinearSegments( PCB_TRACK* aSeg1, PCB_TRACK* aSeg2 )
 {
     if( aSeg1->IsLocked() || aSeg2->IsLocked() )
         return false;
 
-    std::shared_ptr<CONNECTIVITY_DATA> connectivity = m_brd->GetConnectivity();
+    // Collect the unique points where the two tracks are connected to other items
-
-    std::vector<BOARD_CONNECTED_ITEM*> tracks = connectivity->GetConnectedItems( aSeg1,
-            { PCB_TRACE_T, PCB_ARC_T, PCB_VIA_T, PCB_PAD_T, PCB_ZONE_T } );
-    std::vector<BOARD_CONNECTED_ITEM*> tracks2 = connectivity->GetConnectedItems( aSeg2,
-            { PCB_TRACE_T, PCB_ARC_T, PCB_VIA_T, PCB_PAD_T, PCB_ZONE_T } );
-
-    std::move( tracks2.begin(), tracks2.end(), std::back_inserter( tracks ) );
-    std::sort( tracks.begin(), tracks.end() );
-    tracks.erase( std::unique( tracks.begin(), tracks.end() ), tracks.end() );
-
-    tracks.erase(
-            std::remove_if( tracks.begin(), tracks.end(), [ aSeg1, aSeg2 ]( BOARD_CONNECTED_ITEM* aTest )
-            {
-                return ( aTest == aSeg1 ) || ( aTest == aSeg2 );
-            } ), tracks.end() );
-
     std::set<VECTOR2I> pts;
 
-    // Collect the unique points where the two tracks are connected to other items
+    auto collectPts =
-    for( BOARD_CONNECTED_ITEM* citem : tracks )
+            [&]( BOARD_CONNECTED_ITEM* citem )
+            {
+                if( citem->Type() == PCB_TRACE_T || citem->Type() == PCB_ARC_T
+                        || citem->Type() == PCB_VIA_T )
+                {
+                    PCB_TRACK* track = static_cast<PCB_TRACK*>( citem );
+
+                    if( track->IsPointOnEnds( aSeg1->GetStart() ) )
+                        pts.emplace( aSeg1->GetStart() );
+
+                    if( track->IsPointOnEnds( aSeg1->GetEnd() ) )
+                        pts.emplace( aSeg1->GetEnd() );
+
+                    if( track->IsPointOnEnds( aSeg2->GetStart() ) )
+                        pts.emplace( aSeg2->GetStart() );
+
+                    if( track->IsPointOnEnds( aSeg2->GetEnd() ) )
+                        pts.emplace( aSeg2->GetEnd() );
+                }
+                else
+                {
+                    if( citem->HitTest( aSeg1->GetStart(), ( aSeg1->GetWidth() + 1 ) / 2 ) )
+                        pts.emplace( aSeg1->GetStart() );
+
+                    if( citem->HitTest( aSeg1->GetEnd(), ( aSeg1->GetWidth() + 1 ) / 2  ) )
+                        pts.emplace( aSeg1->GetEnd() );
+
+                    if( citem->HitTest( aSeg2->GetStart(), ( aSeg2->GetWidth() + 1 ) / 2  ) )
+                        pts.emplace( aSeg2->GetStart() );
+
+                    if( citem->HitTest( aSeg2->GetEnd(), ( aSeg2->GetWidth() + 1 ) / 2  ) )
+                        pts.emplace( aSeg2->GetEnd() );
+                }
+            };
+
+    for( BOARD_CONNECTED_ITEM* item : getConnectedItems( aSeg1 ) )
     {
-
+        if( item != aSeg1 && item != aSeg2 )
-        if( PCB_TRACK* track = dyn_cast<PCB_TRACK*>( citem ) )
+            collectPts( item );
-        {
-            if( track->IsPointOnEnds( aSeg1->GetStart() ) )
-                pts.emplace( aSeg1->GetStart() );
-
-            if( track->IsPointOnEnds( aSeg1->GetEnd() ) )
-                pts.emplace( aSeg1->GetEnd() );
-
-            if( track->IsPointOnEnds( aSeg2->GetStart() ) )
-                pts.emplace( aSeg2->GetStart() );
-
-            if( track->IsPointOnEnds( aSeg2->GetEnd() ) )
-                pts.emplace( aSeg2->GetEnd() );
-        }
-        else
-        {
-            if( citem->HitTest( aSeg1->GetStart(), ( aSeg1->GetWidth() + 1 ) / 2 ) )
-                pts.emplace( aSeg1->GetStart() );
-
-            if( citem->HitTest( aSeg1->GetEnd(), ( aSeg1->GetWidth() + 1 ) / 2  ) )
-                pts.emplace( aSeg1->GetEnd() );
-
-            if( citem->HitTest( aSeg2->GetStart(), ( aSeg2->GetWidth() + 1 ) / 2  ) )
-                pts.emplace( aSeg2->GetStart() );
-
-            if( citem->HitTest( aSeg2->GetEnd(), ( aSeg2->GetWidth() + 1 ) / 2  ) )
-                pts.emplace( aSeg2->GetEnd() );
-        }
     }
 
+    for( BOARD_CONNECTED_ITEM* item : getConnectedItems( aSeg2 ) )
+    {
+        if( item != aSeg1 && item != aSeg2 )
+            collectPts( item );
+    }
 
     // This means there is a node in the center
     if( pts.size() > 2 )
@@ -613,10 +654,10 @@ bool TRACKS_CLEANER::mergeCollinearSegments( PCB_TRACK* aSeg1, PCB_TRACK* aSeg2
         m_commit.Modify( aSeg1 );
         *aSeg1 = dummy_seg;
 
-        connectivity->Update( aSeg1 );
+        m_brd->GetConnectivity()->Update( aSeg1 );
 
         // Clear the status flags here after update.
-        for( auto pad : connectivity->GetConnectedPads( aSeg1 ) )
+        for( PAD* pad : m_brd->GetConnectivity()->GetConnectedPads( aSeg1 ) )
         {
             aSeg1->SetState( BEGIN_ONPAD, pad->HitTest( aSeg1->GetStart() ) );
             aSeg1->SetState( END_ONPAD, pad->HitTest( aSeg1->GetEnd() ) );

pcbnew/tracks_cleaner.h

@@ -98,12 +98,17 @@ private:
 
     void removeItems( std::set<BOARD_ITEM*>& aItems );
 
+    const std::vector<BOARD_CONNECTED_ITEM*>& getConnectedItems( PCB_TRACK* aTrack );
+
 private:
     BOARD*                                      m_brd;
     BOARD_COMMIT&                               m_commit;       // caller owns
     bool                                        m_dryRun;
     std::vector<std::shared_ptr<CLEANUP_ITEM>>* m_itemsList;    // caller owns
     REPORTER*                                   m_reporter;
+
+    // Cache connections.  O(n^2) is awful, but it beats O(2n^3).
+    std::map<PCB_TRACK*, std::vector<BOARD_CONNECTED_ITEM*>> m_connectedItemsCache;
 };