From 4abee7c2eb9f0eb968aaf01450cc7abffb0d43e6 Mon Sep 17 00:00:00 2001
From: Roberto Fernandez Bautista <roberto.fer.bau@gmail.com>
Date: Sat, 25 Sep 2021 19:36:51 +0100
Subject: [PATCH] Fix SHAPE_LINE_CHAIN::Split when inserting a point on an arc
 Ensure the arc is split into two at the point specified.

Partly fixes https://gitlab.com/kicad/code/kicad/-/issues/9023
Additional work required to ensure that the shove state is preserved.
---
 libs/kimath/src/geometry/shape_line_chain.cpp | 23 ++++--
 .../kimath/geometry/test_shape_line_chain.cpp | 74 +++++++++++++++++++
 2 files changed, 90 insertions(+), 7 deletions(-)

diff --git a/libs/kimath/src/geometry/shape_line_chain.cpp b/libs/kimath/src/geometry/shape_line_chain.cpp
index a7b0eedb49..f51f6d58ee 100644
--- a/libs/kimath/src/geometry/shape_line_chain.cpp
+++ b/libs/kimath/src/geometry/shape_line_chain.cpp
@@ -652,15 +652,24 @@ int SHAPE_LINE_CHAIN::Split( const VECTOR2I& aP )
 
     if( ii >= 0 )
     {
-        // Are we splitting at the beginning of an arc?  If so, let's split right before so that
-        // the shape is preserved
+        // Don't create duplicate points
+        if( GetPoint( ii ) == aP )
+            return ii;
+
+        size_t newIndex = static_cast<size_t>( ii ) + 1;
+
         if( IsArcSegment( ii ) )
-            ii--;
+        {
+            m_points.insert( m_points.begin() + newIndex, aP );
+            m_shapes.insert( m_shapes.begin() + newIndex, { ArcIndex( ii ), SHAPE_IS_PT } );
+            splitArc( newIndex, true ); // Make the inserted point a shared point
+        }
+        else
+        {
+            Insert( newIndex, aP );
+        }
 
-        m_points.insert( m_points.begin() + ( ii + 1 ), aP );
-        m_shapes.insert( m_shapes.begin() + ( ii + 1 ), SHAPES_ARE_PT );
-
-        return ii + 1;
+        return newIndex;
     }
 
     return -1;
diff --git a/qa/libs/kimath/geometry/test_shape_line_chain.cpp b/qa/libs/kimath/geometry/test_shape_line_chain.cpp
index 66856628f8..2646de9b8e 100644
--- a/qa/libs/kimath/geometry/test_shape_line_chain.cpp
+++ b/qa/libs/kimath/geometry/test_shape_line_chain.cpp
@@ -108,6 +108,80 @@ BOOST_AUTO_TEST_CASE( ArcToPolylineLargeCoords )
 }
 
 
+// Test SHAPE_LINE_CHAIN::Split()
+BOOST_AUTO_TEST_CASE( Split )
+{
+    SEG       seg1( VECTOR2I( 0, 100000 ), VECTOR2I( 50000, 0 ) );
+    SEG       seg2( VECTOR2I( 200000, 0 ), VECTOR2I( 300000, 0 ) );
+    SHAPE_ARC arc( VECTOR2I( 200000, 0 ), VECTOR2I( 300000, 0 ), 180.0 );
+
+    // Start a chain with 2 points (seg1)
+    SHAPE_LINE_CHAIN chain( { seg1.A, seg1.B } );
+    BOOST_CHECK_EQUAL( chain.PointCount(), 2 );
+    // Add first arc
+    chain.Append( arc );
+    BOOST_CHECK_EQUAL( chain.PointCount(), 9 );
+    // Add two points (seg2)
+    chain.Append( seg2.A );
+    chain.Append( seg2.B );
+    BOOST_CHECK_EQUAL( chain.PointCount(), 11 );
+    BOOST_CHECK( GEOM_TEST::IsOutlineValid( chain ) );
+
+    BOOST_TEST_CONTEXT( "Case 1: Point not in the chain" )
+    {
+        SHAPE_LINE_CHAIN chainCopy = chain;
+        BOOST_CHECK_EQUAL( chainCopy.Split( VECTOR2I( 400000, 0 ) ), -1 );
+        BOOST_CHECK_EQUAL( chainCopy.PointCount(), chain.PointCount() );
+        BOOST_CHECK_EQUAL( chainCopy.ArcCount(), chain.ArcCount() );
+    }
+
+    BOOST_TEST_CONTEXT( "Case 2: Point close to start of a segment" )
+    {
+        SHAPE_LINE_CHAIN chainCopy = chain;
+        VECTOR2I         splitPoint = seg1.A + VECTOR2I( 5, -10 );
+        BOOST_CHECK_EQUAL( chainCopy.Split( splitPoint ), 1 );
+        BOOST_CHECK( GEOM_TEST::IsOutlineValid( chainCopy ) );
+        BOOST_CHECK_EQUAL( chainCopy.GetPoint( 1 ), splitPoint );
+        BOOST_CHECK_EQUAL( chainCopy.PointCount(), chain.PointCount() + 1 ); // new point added
+        BOOST_CHECK_EQUAL( chainCopy.ArcCount(), chain.ArcCount() );
+    }
+
+    BOOST_TEST_CONTEXT( "Case 3: Point exactly on the segment" )
+    {
+        SHAPE_LINE_CHAIN chainCopy = chain;
+        VECTOR2I         splitPoint = seg1.B;
+        BOOST_CHECK_EQUAL( chainCopy.Split( splitPoint ), 1 );
+        BOOST_CHECK( GEOM_TEST::IsOutlineValid( chainCopy ) );
+        BOOST_CHECK_EQUAL( chainCopy.GetPoint( 1 ), splitPoint );
+        BOOST_CHECK_EQUAL( chainCopy.PointCount(), chain.PointCount() );
+        BOOST_CHECK_EQUAL( chainCopy.ArcCount(), chain.ArcCount() );
+    }
+
+    BOOST_TEST_CONTEXT( "Case 4: Point at start of arc" )
+    {
+        SHAPE_LINE_CHAIN chainCopy = chain;
+        VECTOR2I         splitPoint = arc.GetP0();
+        BOOST_CHECK_EQUAL( chainCopy.Split( splitPoint ), 2 );
+        BOOST_CHECK( GEOM_TEST::IsOutlineValid( chainCopy ) );
+        BOOST_CHECK_EQUAL( chainCopy.GetPoint( 2 ), splitPoint );
+        BOOST_CHECK_EQUAL( chainCopy.PointCount(), chain.PointCount() );
+        BOOST_CHECK_EQUAL( chainCopy.ArcCount(), chain.ArcCount() );
+    }
+
+    BOOST_TEST_CONTEXT( "Case 5: Point close to start of arc" )
+    {
+        SHAPE_LINE_CHAIN chainCopy = chain;
+        VECTOR2I         splitPoint = arc.GetP0() + VECTOR2I( -10, 130 );
+        BOOST_CHECK_EQUAL( chainCopy.Split( splitPoint ), 3 );
+        BOOST_CHECK( GEOM_TEST::IsOutlineValid( chainCopy ) );
+        BOOST_CHECK_EQUAL( chainCopy.GetPoint( 3 ), splitPoint );
+        BOOST_CHECK_EQUAL( chainCopy.IsSharedPt( 3 ), true ); // must be a shared point
+        BOOST_CHECK_EQUAL( chainCopy.PointCount(), chain.PointCount() + 1 ); // new point added
+        BOOST_CHECK_EQUAL( chainCopy.ArcCount(), chain.ArcCount() + 1 ); // new arc should have been created
+    }
+}
+
+
 // Test SHAPE_LINE_CHAIN::Slice()
 BOOST_AUTO_TEST_CASE( Slice )
 {