From c4c8848fa39079dccbc75d0f74333384c07fe3e1 Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Fri, 18 Mar 2022 11:04:17 +0000 Subject: [PATCH] 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 | 25 ++- pcbnew/cleanup_item.h | 3 +- .../dialog_cleanup_tracks_and_vias.cpp | 11 +- pcbnew/tracks_cleaner.cpp | 159 +++++++++++------- pcbnew/tracks_cleaner.h | 5 + 5 files changed, 122 insertions(+), 81 deletions(-) diff --git a/pcbnew/cleanup_item.cpp b/pcbnew/cleanup_item.cpp index c609fa1805..fd31a15dbf 100644 --- a/pcbnew/cleanup_item.cpp +++ b/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_VIA: msg = _HKI( "Remove via shorting two nets" ); break; - case CLEANUP_REDUNDANT_VIA: msg = _HKI( "Remove redundant via" ); break; - case CLEANUP_DUPLICATE_TRACK: msg = _HKI( "Remove duplicate track" ); 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_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_TRACK_IN_PAD: msg = _HKI( "Remove track inside pad" ); 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_REDUNDANT_VIA: msg = _HKI( "Remove redundant via" ); break; + case CLEANUP_DUPLICATE_TRACK: msg = _HKI( "Remove duplicate track" ); 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_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_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_DUPLICATE_GRAPHIC: msg = _HKI( "Remove duplicated graphic" ); break; - case CLEANUP_LINES_TO_RECT: msg = _HKI( "Convert lines to rectangle" ); break; + case CLEANUP_NULL_GRAPHIC: msg = _HKI( "Remove zero-size graphic" ); break; + case CLEANUP_DUPLICATE_GRAPHIC: msg = _HKI( "Remove duplicated graphic" ); break; + case CLEANUP_LINES_TO_RECT: msg = _HKI( "Convert lines to rectangle" ); break; default: wxFAIL_MSG( wxT( "Missing cleanup item description" ) ); diff --git a/pcbnew/cleanup_item.h b/pcbnew/cleanup_item.h index acdba779e8..2bbd4f01ff 100644 --- a/pcbnew/cleanup_item.h +++ b/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_SHORTING_TRACK = CLEANUP_FIRST, CLEANUP_SHORTING_VIA, CLEANUP_REDUNDANT_VIA, CLEANUP_DUPLICATE_TRACK, diff --git a/pcbnew/dialogs/dialog_cleanup_tracks_and_vias.cpp b/pcbnew/dialogs/dialog_cleanup_tracks_and_vias.cpp index 1ba2ca3510..7f5833f81f 100644 --- a/pcbnew/dialogs/dialog_cleanup_tracks_and_vias.cpp +++ b/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_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()->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 ) { diff --git a/pcbnew/tracks_cleaner.cpp b/pcbnew/tracks_cleaner.cpp index 18ceb2d121..5316148057 100644 --- a/pcbnew/tracks_cleaner.cpp +++ b/pcbnew/tracks_cleaner.cpp @@ -62,20 +62,29 @@ void TRACKS_CLEANER::CleanupBoard( bool aDryRun, std::vectorReport( _( "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::vectorReport( _( "Delete tracks in pads" ) ); + wxSafeYield(); // Timeslice to update UI + } deleteTracksInPads(); } @@ -93,7 +105,10 @@ void TRACKS_CLEANER::CleanupBoard( bool aDryRun, std::vectorReport( _( "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 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( candidateItem ); + PCB_TRACK* candidateSegment = static_cast( 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 |= mergeCollinearSegments( segment, candidate ); + merged = true; + break; } } } @@ -485,64 +514,76 @@ void TRACKS_CLEANER::cleanup( bool aDeleteDuplicateVias, bool aDeleteNullSegment } +const std::vector& TRACKS_CLEANER::getConnectedItems( PCB_TRACK* aTrack ) +{ + const std::shared_ptr& 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 = m_brd->GetConnectivity(); - - std::vector tracks = connectivity->GetConnectedItems( aSeg1, - { PCB_TRACE_T, PCB_ARC_T, PCB_VIA_T, PCB_PAD_T, PCB_ZONE_T } ); - std::vector 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() ); - + // Collect the unique points where the two tracks are connected to other items std::set pts; - // Collect the unique points where the two tracks are connected to other items - for( BOARD_CONNECTED_ITEM* citem : tracks ) + auto collectPts = + [&]( 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( 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( PCB_TRACK* track = dyn_cast( 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() ); - } + if( item != aSeg1 && item != aSeg2 ) + collectPts( item ); } + 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() ) ); diff --git a/pcbnew/tracks_cleaner.h b/pcbnew/tracks_cleaner.h index 00483c5cf1..5d5b602ceb 100644 --- a/pcbnew/tracks_cleaner.h +++ b/pcbnew/tracks_cleaner.h @@ -98,12 +98,17 @@ private: void removeItems( std::set& aItems ); + const std::vector& getConnectedItems( PCB_TRACK* aTrack ); + private: BOARD* m_brd; BOARD_COMMIT& m_commit; // caller owns bool m_dryRun; std::vector>* m_itemsList; // caller owns REPORTER* m_reporter; + + // Cache connections. O(n^2) is awful, but it beats O(2n^3). + std::map> m_connectedItemsCache; };