SHAPE_LINE_CHAIN: Fix and simplify NextShape() + remove PrevShape()

NextShape() function was too complex (trying to go back and forwards).
We only used PrevShape() in one place, so removed that usage.

Added QA tests

libs/kimath/include/geometry/shape_line_chain.h

@@ -368,16 +368,10 @@ public:
      * the arc, in other words, the last point of the arc.
      *
      * @param aPointIndex is a vertex in the chain.
-     * @param aForwards is true if the next shape is desired, false for previous shape.
      * @return the vertex index of the start of the next shape after aPoint's shape or -1 if
      * the end was reached.
      */
-    int NextShape( int aPointIndex, bool aForwards = true ) const;
+    int NextShape( int aPointIndex ) const;
-
-    int PrevShape( int aPointIndex ) const
-    {
-        return NextShape( aPointIndex, false );
-    }
 
     /**
      * Move a point to a specific location.
@@ -864,15 +858,14 @@ public:
 
         if( nextIdx > m_shapes.size() - 1 )
         {
-            if( nextIdx == m_shapes.size() && m_closed )
+            if( nextIdx == m_shapes.size() && m_closed && IsSharedPt( 0 ) )
                 nextIdx = 0; // segment between end point and first point
             else
                 return false;
         }
 
         return ( IsPtOnArc( aSegment )
-                 && ( IsSharedPt( aSegment )
+                 && ( ArcIndex( aSegment ) == m_shapes[nextIdx].first ) );
-                      || m_shapes[aSegment].first == m_shapes[nextIdx].first ) );
     }
 
 

libs/kimath/src/geometry/shape_line_chain.cpp

@@ -1028,60 +1028,49 @@ int SHAPE_LINE_CHAIN::ShapeCount() const
 }
 
 
-int SHAPE_LINE_CHAIN::NextShape( int aPointIndex, bool aForwards ) const
+int SHAPE_LINE_CHAIN::NextShape( int aPointIndex ) const
 {
     if( aPointIndex < 0 )
         aPointIndex += PointCount();
 
+    if( aPointIndex < 0 )
+        return -1;
+
     int lastIndex = PointCount() - 1;
 
-    // First or last point?
+    // Last point?
-    if( ( aForwards && aPointIndex == lastIndex ) ||
+    if( aPointIndex >= lastIndex )
-        ( !aForwards && aPointIndex == 0 ) )
-    {
         return -1; // we don't want to wrap around
-    }
-
-    int delta = aForwards ? 1 : -1;
 
     if( m_shapes[aPointIndex] == SHAPES_ARE_PT )
-        return aPointIndex + delta;
+        return aPointIndex + 1;
 
     int arcStart = aPointIndex;
 
     // The second element should only get populated when the point is shared between two shapes.
     // If not a shared point, then the index should always go on the first element.
-    assert( m_shapes[aPointIndex].first != SHAPE_IS_PT );
+    wxCHECK2_MSG( m_shapes[aPointIndex].first != SHAPE_IS_PT, return -1, "malformed chain!" );
 
-    // Start with the assumption the point is shared
+    ssize_t currentArcIdx = ArcIndex( aPointIndex );
-    auto arcIndex = [&]( int aIndex ) -> ssize_t
-                    {
-                        if( aForwards )
-                            return ArcIndex( aIndex );
-                        else
-                            return reversedArcIndex( aIndex );
-                    };
-
-    ssize_t currentArcIdx = arcIndex( aPointIndex );
 
     // Now skip the rest of the arc
-    while( aPointIndex < lastIndex && aPointIndex >= 0 && arcIndex( aPointIndex ) == currentArcIdx )
+    while( aPointIndex < lastIndex && ArcIndex( aPointIndex ) == currentArcIdx )
-        aPointIndex += delta;
+        aPointIndex += 1;
-
-    if( aPointIndex == lastIndex )
-    {
-        if( !m_closed && arcIndex( aPointIndex ) == currentArcIdx )
-            return -1;
-        else
-            return lastIndex; // Segment between last point and the start
-    }
 
     bool indexStillOnArc = alg::pair_contains( m_shapes[aPointIndex], currentArcIdx );
 
     // We want the last vertex of the arc if the initial point was the start of one
     // Well-formed arcs should generate more than one point to travel above
     if( aPointIndex - arcStart > 1 && !indexStillOnArc )
-        aPointIndex -= delta;
+        aPointIndex -= 1;
+
+    if( aPointIndex == lastIndex )
+    {
+        if( !m_closed || IsArcSegment( aPointIndex ) )
+            return -1; //no shape
+        else
+            return lastIndex; // Segment between last point and the start of the chain
+    }
 
     return aPointIndex;
 }

pcbnew/router/pns_line_placer.cpp

@@ -217,9 +217,7 @@ bool LINE_PLACER::handlePullback()
 
     if( pullback_1 || pullback_2 )
     {
-        lastSegIdx = tail.PrevShape( -1 );
+        if( !tail.IsArcSegment( lastSegIdx ) )
-
-        if( !tail.IsPtOnArc( lastSegIdx ) )
         {
             const SEG& seg = tail.CSegment( lastSegIdx );
             m_direction    = DIRECTION_45( seg );

qa/tests/libs/kimath/geometry/test_shape_line_chain.cpp

@@ -35,7 +35,78 @@
  * NOTE: Collision of SHAPE_LINE_CHAIN with arcs is tested in test_shape_arc.cpp
  */
 
-BOOST_AUTO_TEST_SUITE( ShapeLineChain )
+struct SLC_CASES
+{
+    SHAPE_LINE_CHAIN Circle1Arc;
+    SHAPE_LINE_CHAIN Circle2Arcs;
+    SHAPE_LINE_CHAIN ArcsCoincident;
+    SHAPE_LINE_CHAIN ArcsCoincidentClosed;
+    SHAPE_LINE_CHAIN ArcsIndependent;
+    SHAPE_LINE_CHAIN DuplicateArcs;
+    SHAPE_LINE_CHAIN ArcsAndSegMixed;
+    SHAPE_LINE_CHAIN ArcAndPoint;
+    SHAPE_LINE_CHAIN EmptyChain;
+    SHAPE_LINE_CHAIN OnePoint;
+
+    SHAPE_ARC ArcCircle; ///< Full Circle arc
+    SHAPE_ARC Arc0a; ///< First half of a circle
+    SHAPE_ARC Arc0b; ///< Second half of a circle
+    SHAPE_ARC Arc1; ///< start coincident with Arc0a end
+    SHAPE_ARC Arc2; ///< Independent arc
+
+    SLC_CASES()
+    {
+        ArcCircle = SHAPE_ARC( VECTOR2I( 183450000, 128360000 ),
+                               VECTOR2I( 183850000, 128360000 ),
+                               VECTOR2I( 183450000, 128360000 ), 0 );
+
+        Arc0a =     SHAPE_ARC( VECTOR2I( 183450000, 128360000 ),
+                               VECTOR2I( 183650000, 128560000 ),
+                               VECTOR2I( 183850000, 128360000 ), 0 );
+
+        Arc0b =     SHAPE_ARC( VECTOR2I( 183850000, 128360000 ),
+                               VECTOR2I( 183650000, 128160000 ),
+                               VECTOR2I( 183450000, 128360000 ), 0 );
+
+        Arc1 =      SHAPE_ARC( VECTOR2I( 183850000, 128360000 ),
+                               VECTOR2I( 183638550, 128640305 ),
+                               VECTOR2I( 183500000, 129204974 ), 0 );
+
+        Arc2 =      SHAPE_ARC( VECTOR2I( 283450000, 228360000 ),
+                               VECTOR2I( 283650000, 228560000 ),
+                               VECTOR2I( 283850000, 228360000 ), 0 );
+
+        Circle1Arc.Append( ArcCircle );
+        Circle1Arc.SetClosed( true );
+
+        Circle2Arcs.Append( Arc0a );
+        Circle2Arcs.Append( Arc0b );
+        Circle2Arcs.SetClosed( true );
+
+        ArcsCoincident.Append( Arc0a );
+        ArcsCoincident.Append( Arc1 );
+
+        ArcsCoincidentClosed=ArcsCoincident;
+        ArcsCoincidentClosed.SetClosed( true );
+
+        ArcsIndependent.Append( Arc0a );
+        ArcsIndependent.Append( Arc2 );
+
+        DuplicateArcs=ArcsCoincident;
+        DuplicateArcs.Append( Arc1 ); //should add a segment between end of the chain and new copy of the arc
+
+        ArcAndPoint.Append( Arc0a );
+        ArcAndPoint.Append( VECTOR2I( 233450000, 228360000 ) );
+
+        ArcsAndSegMixed = ArcAndPoint;
+        ArcsAndSegMixed.Append( Arc2 );
+
+        OnePoint.Append( VECTOR2I( 233450000, 228360000 ) );
+    }
+};
+
+
+BOOST_FIXTURE_TEST_SUITE( TestShapeLineChain, SLC_CASES )
 
 BOOST_AUTO_TEST_CASE( ArcToPolyline )
 {
@@ -156,6 +227,50 @@ BOOST_AUTO_TEST_CASE( SimplifyDuplicatePoint )
 }
 
 
+
+
+BOOST_AUTO_TEST_CASE( NextShape )
+{
+    BOOST_CHECK_EQUAL( Circle1Arc.NextShape( 0 ), -1 ); //only one arc
+    
+    BOOST_CHECK_EQUAL( Circle2Arcs.NextShape( 0 ), 8 ); // next shape "Arc0b"
+    BOOST_CHECK_EQUAL( Circle2Arcs.NextShape( 8 ), -1 ); //no more shapes (last point joins with first, part of arc)
+
+    BOOST_CHECK_EQUAL( ArcsCoincident.NextShape( 0 ), 8 ); // next shape "Arc1"
+    BOOST_CHECK_EQUAL( ArcsCoincident.NextShape( 8 ), -1 ); //no more shapes
+
+    BOOST_CHECK_EQUAL( ArcsCoincidentClosed.NextShape( 0 ), 8 ); // next shape "Arc1"
+    BOOST_CHECK_EQUAL( ArcsCoincidentClosed.NextShape( 8 ), 13 ); //next shape is hidden segment joining last/first
+    BOOST_CHECK_EQUAL( ArcsCoincidentClosed.NextShape( 13 ), -1 ); //no more shapes
+
+    BOOST_CHECK_EQUAL( DuplicateArcs.NextShape( 0 ), 8 ); // next shape "Arc1"
+    BOOST_CHECK_EQUAL( DuplicateArcs.NextShape( 8 ), 13 ); // next shape hidden segment joining the 2 duplicate arcs
+    BOOST_CHECK_EQUAL( DuplicateArcs.NextShape( 13 ), 14 ); // next shape "Arc1" (duplicate)
+    BOOST_CHECK_EQUAL( DuplicateArcs.NextShape( 14 ), -1 ); //no more shapes
+
+    BOOST_CHECK_EQUAL( ArcAndPoint.NextShape( 0 ), 8 ); // next shape straight segment (end of arc->point)
+    BOOST_CHECK_EQUAL( ArcAndPoint.NextShape( 8 ), -1 ); //no more shapes
+
+    BOOST_CHECK_EQUAL( ArcsAndSegMixed.NextShape( 0 ), 8 ); // next shape straight segment (end of arc->point)
+    BOOST_CHECK_EQUAL( ArcsAndSegMixed.NextShape( 8 ), 9 ); // next shape straight segment (point->begining of arc)
+    BOOST_CHECK_EQUAL( ArcsAndSegMixed.NextShape( 9 ), 10 ); //next shape second arc
+    BOOST_CHECK_EQUAL( ArcsAndSegMixed.NextShape( 10 ), -1 ); //no more shapes
+    BOOST_CHECK_EQUAL( ArcsAndSegMixed.NextShape( 20 ), -1 ); //invalid indices should still work
+    BOOST_CHECK_EQUAL( ArcsAndSegMixed.NextShape( -50 ), -1 ); //invalid indices should still work
+
+    BOOST_CHECK_EQUAL( EmptyChain.NextShape( 0 ), -1 );
+    BOOST_CHECK_EQUAL( EmptyChain.NextShape( 1 ), -1 ); //invalid indices should still work
+    BOOST_CHECK_EQUAL( EmptyChain.NextShape( 2 ), -1 ); //invalid indices should still work
+    BOOST_CHECK_EQUAL( EmptyChain.NextShape( -2 ), -1 ); //invalid indices should still work
+
+    BOOST_CHECK_EQUAL( OnePoint.NextShape( 0 ), -1 );
+    BOOST_CHECK_EQUAL( OnePoint.NextShape( -1 ), -1 );
+    BOOST_CHECK_EQUAL( OnePoint.NextShape( 1 ), -1 );  //invalid indices should still work
+    BOOST_CHECK_EQUAL( OnePoint.NextShape( 2 ), -1 ); //invalid indices should still work
+    BOOST_CHECK_EQUAL( OnePoint.NextShape( -2 ), -1 ); //invalid indices should still work
+}
+
+
 // Test special case where the last arc in the chain has a shared point with the first arc
 BOOST_AUTO_TEST_CASE( ArcWrappingToStartSharedPoints )
 {