Refactor gfx import cleanup

Break up monolithic function into responsibilities.  Adjust cleanup to
correctly modify each graphical pairing.  Fix drc test to properly
report gap distances that are relevant to outlines

Fixes https://gitlab.com/kicad/code/kicad/-/issues/13090

pcbnew/drc/drc_test_provider_misc.cpp

@@ -147,21 +147,15 @@ void DRC_TEST_PROVIDER_MISC::testOutline()
                 if( !itemA )        // If we only have a single item, make sure it's A
                     std::swap( itemA, itemB );
 
-                VECTOR2I markerPos = pt;
-                int      gap = 0;
+                VECTOR2I   markerPos = pt;
+                int        gap = 0;
+                PCB_SHAPE* shapeA = nullptr;
+                PCB_SHAPE* shapeB = nullptr;
 
-                if( itemA && itemB )
+                if( itemA && itemB && itemA->Type() == PCB_SHAPE_T && itemB->Type() == PCB_SHAPE_T )
                 {
-                    std::shared_ptr<SHAPE> shapeA = itemA->GetEffectiveShape();
-                    std::shared_ptr<SHAPE> shapeB = itemB->GetEffectiveShape();
-                    VECTOR2I            ptA;
-                    VECTOR2I            ptB;
-
-                    if( shapeA && shapeB && shapeA->NearestPoints( shapeB.get(), ptA, ptB ) )
-                    {
-                        gap = ( ptA - ptB ).EuclideanNorm();
-                        markerPos = ( ptA + ptB ) / 2;
-                    }
+                    shapeA = static_cast<PCB_SHAPE*>( itemA );
+                    shapeB = static_cast<PCB_SHAPE*>( itemB );
                 }
                 else
                 {
@@ -174,9 +168,24 @@ void DRC_TEST_PROVIDER_MISC::testOutline()
                     itemB = shapeB;
                 }
 
-                std::shared_ptr<DRC_ITEM> drcItem = DRC_ITEM::Create( DRCE_INVALID_OUTLINE );
+                if( shapeA && shapeB )
+                {
+                    VECTOR2I pts0[2] = { shapeA->GetStart(), shapeA->GetEnd() };
+                    VECTOR2I pts1[2] = { shapeB->GetStart(), shapeB->GetEnd() };
+
+                    SEG::ecoord d[4];
+                    d[0] = ( pts0[0] - pts1[0] ).SquaredEuclideanNorm();
+                    d[1] = ( pts0[0] - pts1[1] ).SquaredEuclideanNorm();
+                    d[2] = ( pts0[1] - pts1[0] ).SquaredEuclideanNorm();
+                    d[3] = ( pts0[1] - pts1[1] ).SquaredEuclideanNorm();
+
+                    int idx = std::min_element( d, d + 4 ) - d;
+                    gap = std::sqrt( d[idx] );
+                    markerPos = ( pts0[idx / 2] + pts1[idx % 2] ) / 2;
+                }
 
-                wxString msg2;
+                std::shared_ptr<DRC_ITEM> drcItem = DRC_ITEM::Create( DRCE_INVALID_OUTLINE );
+                wxString                  msg2;
 
                 if( itemA && itemB )
                     msg2.Printf( _( "%s (gap %s)" ), msg, MessageTextFromValue( gap ) );

pcbnew/fix_board_shape.cpp

@@ -26,79 +26,110 @@
 
 #include <vector>
 #include <algorithm>
+#include <limits>
 #include <pcb_shape.h>
 #include <geometry/circle.h>
 
+#include <nanoflann.hpp>
+
+
+struct PCB_SHAPE_ENDPOINTS_ADAPTOR
+{
+    std::vector<std::pair<VECTOR2I, PCB_SHAPE*>> endpoints;
+
+    PCB_SHAPE_ENDPOINTS_ADAPTOR( const std::vector<PCB_SHAPE*>& shapes )
+    {
+        endpoints.reserve( shapes.size() * 2 );
+
+        for( PCB_SHAPE* shape : shapes )
+        {
+            endpoints.emplace_back( shape->GetStart(), shape );
+            endpoints.emplace_back( shape->GetEnd(), shape );
+        }
+    }
+
+    // Required by nanoflann
+    size_t kdtree_get_point_count() const { return endpoints.size(); }
+
+    // Returns the dim'th component of the idx'th point
+    double kdtree_get_pt( const size_t idx, const size_t dim ) const
+    {
+        if( dim == 0 )
+            return static_cast<double>( endpoints[idx].first.x );
+        else
+            return static_cast<double>( endpoints[idx].first.y );
+    }
+
+    template <class BBOX>
+    bool kdtree_get_bbox( BBOX& ) const
+    {
+        return false;
+    }
+};
+
+using KDTree = nanoflann::KDTreeSingleIndexAdaptor<nanoflann::L2_Simple_Adaptor<double, PCB_SHAPE_ENDPOINTS_ADAPTOR>,
+                                                   PCB_SHAPE_ENDPOINTS_ADAPTOR,
+                                                   2 /* dim */ >;
+
 
 /**
  * Searches for a PCB_SHAPE matching a given end point or start point in a list.
  * @param aShape The starting shape.
  * @param aPoint The starting or ending point to search for.
- * @param aList The list to remove from.
- * @param aLimit is the distance from \a aPoint that still constitutes a valid find.
+ * @param kdTree The KD-tree for efficient nearest neighbor search.
+ * @param adaptor The adaptor containing the endpoints data.
+ * @param aChainingEpsilon is the distance from \a aPoint that still constitutes a valid find.
  * @return PCB_SHAPE* - The first PCB_SHAPE that has a start or end point matching
  *   aPoint, otherwise NULL if none.
  */
-static PCB_SHAPE* findNext( PCB_SHAPE* aShape, const VECTOR2I& aPoint,
-                            const std::vector<PCB_SHAPE*>& aList, unsigned aLimit )
+static PCB_SHAPE* findNext( PCB_SHAPE* aShape, const VECTOR2I& aPoint, const KDTree& kdTree,
+                            const PCB_SHAPE_ENDPOINTS_ADAPTOR& adaptor, double aChainingEpsilon )
 {
-    // Look for an unused, exact hit
-    for( PCB_SHAPE* graphic : aList )
-    {
-        if( graphic == aShape || ( graphic->GetFlags() & SKIP_STRUCT ) != 0 )
-            continue;
+    const double query_pt[2] = { static_cast<double>( aPoint.x ), static_cast<double>( aPoint.y ) };
 
-        if( aPoint == graphic->GetStart() || aPoint == graphic->GetEnd() )
-            return graphic;
-    }
+    uint32_t indices[2];
+    double distances[2];
+    kdTree.knnSearch( query_pt, 2, indices, distances );
+
+    if( distances[0] == std::numeric_limits<double>::max() )
+        return nullptr;
 
-    // Search again for anything that's close.
-    VECTOR2I    pt( aPoint );
-    SEG::ecoord closest_dist_sq = SEG::Square( aLimit );
-    PCB_SHAPE*  closest_graphic = nullptr;
-    SEG::ecoord d_sq;
+    // Find the closest valid candidate
+    PCB_SHAPE* closest_graphic = nullptr;
+    double closest_dist_sq = aChainingEpsilon * aChainingEpsilon;
 
-    for( PCB_SHAPE* graphic : aList )
+    for( size_t i = 0; i < 2; ++i )
     {
-        if( graphic == aShape || ( graphic->GetFlags() & SKIP_STRUCT ) != 0 )
+        if( distances[i] == std::numeric_limits<double>::max() )
             continue;
 
-        d_sq = ( pt - graphic->GetStart() ).SquaredEuclideanNorm();
+        PCB_SHAPE* candidate = adaptor.endpoints[indices[i]].second;
 
-        if( d_sq < closest_dist_sq )
-        {
-            closest_dist_sq = d_sq;
-            closest_graphic = graphic;
-        }
+        if( candidate == aShape )
+            continue;
 
-        d_sq = ( pt - graphic->GetEnd() ).SquaredEuclideanNorm();
+        if( candidate->GetFlags() & SKIP_STRUCT )
+            continue;
 
-        if( d_sq < closest_dist_sq )
+        if( distances[i] < closest_dist_sq )
         {
-            closest_dist_sq = d_sq;
-            closest_graphic = graphic;
+            closest_dist_sq = distances[i];
+            closest_graphic = candidate;
         }
     }
 
-    return closest_graphic; // Note: will be nullptr if nothing within aLimit
+    return closest_graphic;
 }
 
 
-void ConnectBoardShapes( std::vector<PCB_SHAPE*>&                 aShapeList,
-                         std::vector<std::unique_ptr<PCB_SHAPE>>& aNewShapes, int aChainingEpsilon )
+void ConnectBoardShapes( std::vector<PCB_SHAPE*>& aShapeList, int aChainingEpsilon )
 {
     if( aShapeList.size() == 0 )
         return;
 
-    (void) aNewShapes;
-
-#if 0
-    // Not used, but not removed, just in case
-    auto close_enough = []( const VECTOR2I& aLeft, const VECTOR2I& aRight, unsigned aLimit ) -> bool
-    {
-        return ( aLeft - aRight ).SquaredEuclideanNorm() <= SEG::Square( aLimit );
-    };
-#endif
+    // Pre-build KD-tree
+    PCB_SHAPE_ENDPOINTS_ADAPTOR adaptor( aShapeList );
+    KDTree                      kdTree( 2, adaptor );
 
     auto closer_to_first = []( const VECTOR2I& aRef, const VECTOR2I& aFirst,
                                const VECTOR2I& aSecond ) -> bool
@@ -288,6 +319,55 @@ void ConnectBoardShapes( std::vector<PCB_SHAPE*>&                 aShapeList,
 
             success = true;
         }
+        else if( shape0 == SHAPE_T::BEZIER  && shape1 == SHAPE_T::BEZIER )
+        {
+            PCB_SHAPE* bez0 = aPrevShape;
+            PCB_SHAPE* bez1 = aShape;
+
+            VECTOR2I pts0[2] = { bez0->GetStart(), bez0->GetEnd() };
+            VECTOR2I pts1[2] = { bez1->GetStart(), bez1->GetEnd() };
+
+            SEG::ecoord d[4];
+            d[0] = ( pts0[0] - pts1[0] ).SquaredEuclideanNorm();
+            d[1] = ( pts0[0] - pts1[1] ).SquaredEuclideanNorm();
+            d[2] = ( pts0[1] - pts1[0] ).SquaredEuclideanNorm();
+            d[3] = ( pts0[1] - pts1[1] ).SquaredEuclideanNorm();
+
+            int idx = std::min_element( d, d + 4 ) - d;
+            int i0 = idx / 2;
+            int i1 = idx % 2;
+            VECTOR2I middle = ( pts0[i0] + pts1[i1] ) / 2;
+
+            // Adjust first bezier curve
+            if( i0 == 0 )
+            {
+                VECTOR2I delta = middle - bez0->GetStart();
+                bez0->SetStart( middle );
+                bez0->SetBezierC1( bez0->GetBezierC1() + delta );
+            }
+            else
+            {
+                VECTOR2I delta = middle - bez0->GetEnd();
+                bez0->SetEnd( middle );
+                bez0->SetBezierC2( bez0->GetBezierC2() + delta );
+            }
+
+            // Adjust second bezier curve
+            if( i1 == 0 )
+            {
+                VECTOR2I delta = middle - bez1->GetStart();
+                bez1->SetStart( middle );
+                bez1->SetBezierC1( bez1->GetBezierC1() + delta );
+            }
+            else
+            {
+                VECTOR2I delta = middle - bez1->GetEnd();
+                bez1->SetEnd( middle );
+                bez1->SetBezierC2( bez1->GetBezierC2() + delta );
+            }
+
+            success = true;
+        }
 
         return success;
     };
@@ -317,7 +397,7 @@ void ConnectBoardShapes( std::vector<PCB_SHAPE*>&                 aShapeList,
             {
                 // Get next closest segment.
                 PCB_SHAPE* nextGraphic =
-                        findNext( curr_graphic, prevPt, aShapeList, aChainingEpsilon );
+                        findNext( curr_graphic, prevPt, kdTree, adaptor, aChainingEpsilon );
 
                 if( !nextGraphic )
                     break;
@@ -336,8 +416,8 @@ void ConnectBoardShapes( std::vector<PCB_SHAPE*>&                 aShapeList,
         const VECTOR2I ptEnd = graphic->GetEnd();
         const VECTOR2I ptStart = graphic->GetStart();
 
-        PCB_SHAPE* grAtEnd = findNext( graphic, ptEnd, aShapeList, aChainingEpsilon );
-        PCB_SHAPE* grAtStart = findNext( graphic, ptStart, aShapeList, aChainingEpsilon );
+        PCB_SHAPE* grAtEnd = findNext( graphic, ptEnd, kdTree, adaptor, aChainingEpsilon );
+        PCB_SHAPE* grAtStart = findNext( graphic, ptStart, kdTree, adaptor, aChainingEpsilon );
 
         bool beginFromEndPt = true;
 

pcbnew/fix_board_shape.h

@@ -32,10 +32,8 @@
 /**
  * Connects shapes to each other, making continious contours (adjacent shapes will have a common vertex)
  * aChainingEpsilon is the max distance between vertices of different shapes to connect.
- * Modifies original shapes, or creates new line segments and stores them in aNewShapes.
+ * Modifies original shapes
  */
-void ConnectBoardShapes( std::vector<PCB_SHAPE*>&                 aShapeList,
-                         std::vector<std::unique_ptr<PCB_SHAPE>>& aNewShapes,
-                         int                                      aChainingEpsilon );
+void ConnectBoardShapes( std::vector<PCB_SHAPE*>& aShapeList, int aChainingEpsilon );
 
 #endif // FIX_BOARD_SHAPE_H_

pcbnew/graphics_cleaner.cpp

@@ -214,7 +214,6 @@ void GRAPHICS_CLEANER::fixBoardOutlines()
         return;
 
     std::vector<PCB_SHAPE*>                 shapeList;
-    std::vector<std::unique_ptr<PCB_SHAPE>> newShapes;
 
     for( BOARD_ITEM* item : m_drawings )
     {
@@ -229,12 +228,9 @@ void GRAPHICS_CLEANER::fixBoardOutlines()
             m_commit.Modify( shape );
     }
 
-    ConnectBoardShapes( shapeList, newShapes, m_outlinesTolerance );
+    ConnectBoardShapes( shapeList, m_outlinesTolerance );
 
     std::vector<PCB_SHAPE*> items_to_select;
-
-    for( std::unique_ptr<PCB_SHAPE>& ptr : newShapes )
-        m_commit.Add( ptr.release() );
 }
 
 

pcbnew/pcb_io/easyeda/pcb_io_easyeda_parser.cpp

@@ -1101,7 +1101,6 @@ FOOTPRINT* PCB_IO_EASYEDA_PARSER::ParseFootprint(
 
     // Heal board outlines
     std::vector<PCB_SHAPE*>                 shapes;
-    std::vector<std::unique_ptr<PCB_SHAPE>> newShapes;
 
     for( BOARD_ITEM* item : footprint->GraphicalItems() )
     {
@@ -1112,10 +1111,7 @@ FOOTPRINT* PCB_IO_EASYEDA_PARSER::ParseFootprint(
             shapes.push_back( static_cast<PCB_SHAPE*>( item ) );
     }
 
-    ConnectBoardShapes( shapes, newShapes, SHAPE_JOIN_DISTANCE );
-
-    for( std::unique_ptr<PCB_SHAPE>& ptr : newShapes )
-        footprint->Add( ptr.release(), ADD_MODE::APPEND );
+    ConnectBoardShapes( shapes, SHAPE_JOIN_DISTANCE );
 
     // EasyEDA footprints don't have courtyard, so build a box ourselves
     if( !footprint->IsOnLayer( F_CrtYd ) )
@@ -1177,7 +1173,6 @@ void PCB_IO_EASYEDA_PARSER::ParseBoard( BOARD* aBoard, const VECTOR2D& aOrigin,
 
     // Heal board outlines
     std::vector<PCB_SHAPE*>                 shapes;
-    std::vector<std::unique_ptr<PCB_SHAPE>> newShapes;
 
     for( BOARD_ITEM* item : aBoard->Drawings() )
     {
@@ -1188,8 +1183,5 @@ void PCB_IO_EASYEDA_PARSER::ParseBoard( BOARD* aBoard, const VECTOR2D& aOrigin,
             shapes.push_back( static_cast<PCB_SHAPE*>( item ) );
     }
 
-    ConnectBoardShapes( shapes, newShapes, SHAPE_JOIN_DISTANCE );
-
-    for( std::unique_ptr<PCB_SHAPE>& ptr : newShapes )
-        aBoard->Add( ptr.release(), ADD_MODE::APPEND );
+    ConnectBoardShapes( shapes, SHAPE_JOIN_DISTANCE );
 }

pcbnew/pcb_io/easyedapro/pcb_io_easyedapro_parser.cpp

@@ -929,7 +929,6 @@ FOOTPRINT* PCB_IO_EASYEDAPRO_PARSER::ParseFootprint( const nlohmann::json&
 
     // Heal board outlines
     std::vector<PCB_SHAPE*>                 edgeShapes;
-    std::vector<std::unique_ptr<PCB_SHAPE>> newShapes;
 
     for( BOARD_ITEM* item : footprint->GraphicalItems() )
     {
@@ -937,10 +936,7 @@ FOOTPRINT* PCB_IO_EASYEDAPRO_PARSER::ParseFootprint( const nlohmann::json&
             edgeShapes.push_back( static_cast<PCB_SHAPE*>( item ) );
     }
 
-    ConnectBoardShapes( edgeShapes, newShapes, SHAPE_JOIN_DISTANCE );
-
-    for( std::unique_ptr<PCB_SHAPE>& ptr : newShapes )
-        footprint->Add( ptr.release(), ADD_MODE::APPEND );
+    ConnectBoardShapes( edgeShapes, SHAPE_JOIN_DISTANCE );
 
     // EasyEDA footprints don't have courtyard, so build a box ourselves
     if( !footprint->IsOnLayer( F_CrtYd ) )
@@ -1859,8 +1855,7 @@ void PCB_IO_EASYEDAPRO_PARSER::ParseBoard(
     }
 
     // Heal board outlines
-    std::vector<PCB_SHAPE*>                 shapes;
-    std::vector<std::unique_ptr<PCB_SHAPE>> newShapes;
+    std::vector<PCB_SHAPE*> shapes;
 
     for( BOARD_ITEM* item : aBoard->Drawings() )
     {
@@ -1871,10 +1866,7 @@ void PCB_IO_EASYEDAPRO_PARSER::ParseBoard(
             shapes.push_back( static_cast<PCB_SHAPE*>( item ) );
     }
 
-    ConnectBoardShapes( shapes, newShapes, SHAPE_JOIN_DISTANCE );
-
-    for( std::unique_ptr<PCB_SHAPE>& ptr : newShapes )
-        aBoard->Add( ptr.release(), ADD_MODE::APPEND );
+    ConnectBoardShapes( shapes, SHAPE_JOIN_DISTANCE );
 
     // Center the board
     BOX2I     outlineBbox = aBoard->ComputeBoundingBox( true );

pcbnew/tools/drawing_tool.cpp

@@ -1850,8 +1850,7 @@ int DRAWING_TOOL::PlaceImportedGraphics( const TOOL_EVENT& aEvent )
 
     if( dlg.ShouldFixDiscontinuities() )
     {
-        std::vector<PCB_SHAPE*>                 shapeList;
-        std::vector<std::unique_ptr<PCB_SHAPE>> newShapes;
+        std::vector<PCB_SHAPE*> shapeList;
 
         for( const std::unique_ptr<EDA_ITEM>& ptr : list )
         {
@@ -1859,13 +1858,7 @@ int DRAWING_TOOL::PlaceImportedGraphics( const TOOL_EVENT& aEvent )
                 shapeList.push_back( shape );
         }
 
-        ConnectBoardShapes( shapeList, newShapes, dlg.GetTolerance() );
-
-        for( std::unique_ptr<PCB_SHAPE>& ptr : newShapes )
-        {
-            ptr->SetParent( m_frame->GetBoard() );
-            list.push_back( std::move( ptr ) );
-        }
+        ConnectBoardShapes( shapeList, dlg.GetTolerance() );
     }
 
     for( std::unique_ptr<EDA_ITEM>& ptr : list )

pcbnew/tools/edit_tool.cpp

@@ -1804,8 +1804,7 @@ int EDIT_TOOL::HealShapes( const TOOL_EVENT& aEvent )
 
     BOARD_COMMIT commit{ this };
 
-    std::vector<PCB_SHAPE*>                 shapeList;
-    std::vector<std::unique_ptr<PCB_SHAPE>> newShapes;
+    std::vector<PCB_SHAPE*> shapeList;
 
     for( EDA_ITEM* item : selection )
     {
@@ -1816,27 +1815,10 @@ int EDIT_TOOL::HealShapes( const TOOL_EVENT& aEvent )
         }
     }
 
-    ConnectBoardShapes( shapeList, newShapes, s_toleranceValue );
-
-    std::vector<PCB_SHAPE*> items_to_select;
-
-    for( std::unique_ptr<PCB_SHAPE>& ptr : newShapes )
-    {
-        PCB_SHAPE* shape = ptr.release();
-
-        commit.Add( shape );
-        items_to_select.push_back( shape );
-    }
+    ConnectBoardShapes( shapeList, s_toleranceValue );
 
     commit.Push( _( "Heal Shapes" ) );
 
-    // Select added items
-    for( PCB_SHAPE* item : items_to_select )
-        m_selectionTool->AddItemToSel( item, true );
-
-    if( items_to_select.size() > 0 )
-        m_toolMgr->ProcessEvent( EVENTS::SelectedEvent );
-
     // Notify other tools of the changes
     m_toolMgr->ProcessEvent( EVENTS::SelectedItemsModified );