From effed5dfdf923c16b59b8fd81eac168e9eb4ff5f Mon Sep 17 00:00:00 2001 From: Roberto Fernandez Bautista Date: Sun, 7 Jan 2024 10:48:08 +0100 Subject: [PATCH] Fix SHAPE_LINE_CHAIN::Remove and SHAPE_LINE_CHAIN::RemoveShape --- libs/kimath/src/geometry/shape_line_chain.cpp | 65 ++++++---- .../kimath/geometry/test_shape_line_chain.cpp | 120 ++++++++++++++++++ 2 files changed, 161 insertions(+), 24 deletions(-) diff --git a/libs/kimath/src/geometry/shape_line_chain.cpp b/libs/kimath/src/geometry/shape_line_chain.cpp index dc9c6f0660..deeb10306c 100644 --- a/libs/kimath/src/geometry/shape_line_chain.cpp +++ b/libs/kimath/src/geometry/shape_line_chain.cpp @@ -821,7 +821,12 @@ void SHAPE_LINE_CHAIN::Replace( int aStartIndex, int aEndIndex, const SHAPE_LINE void SHAPE_LINE_CHAIN::Remove( int aStartIndex, int aEndIndex ) { - assert( m_shapes.size() == m_points.size() ); + wxCHECK( m_shapes.size() == m_points.size(), /*void*/ ); + + // Unwrap the chain first (correctly handling removing arc at + // end of chain coincident with start) + bool closedState = IsClosed(); + SetClosed( false ); if( aEndIndex < 0 ) aEndIndex += PointCount(); @@ -829,19 +834,31 @@ void SHAPE_LINE_CHAIN::Remove( int aStartIndex, int aEndIndex ) if( aStartIndex < 0 ) aStartIndex += PointCount(); - if( aStartIndex >= PointCount() ) + if( aStartIndex >= PointCount() || aEndIndex >= PointCount() || aStartIndex > aEndIndex) + { + SetClosed( closedState ); return; + } - aEndIndex = std::min( aEndIndex, PointCount() - 1 ); - // Split arcs at start index and end just after the end index - if( IsPtOnArc( aStartIndex ) ) - splitArc( aStartIndex ); + // Split arcs, making arcs coincident + if( !IsArcStart( aStartIndex ) && IsPtOnArc( aStartIndex ) ) + splitArc( aStartIndex, false ); - size_t nextIndex = static_cast( aEndIndex ) + 1; + if( IsSharedPt( aStartIndex ) ) // Don't delete the shared point + aStartIndex += 1; - if( IsPtOnArc( nextIndex ) ) - splitArc( nextIndex ); + if( !IsArcEnd( aEndIndex ) && IsPtOnArc( aEndIndex ) && aEndIndex < PointCount() - 1 ) + splitArc( aEndIndex + 1, true ); + + if( IsSharedPt( aEndIndex ) ) // Don't delete the shared point + aEndIndex -= 1; + + if( aStartIndex > aEndIndex ) + { + SetClosed( closedState ); + return; + } std::set extra_arcs; auto logArcIdxRemoval = [&]( ssize_t& aShapeIndex ) @@ -869,14 +886,16 @@ void SHAPE_LINE_CHAIN::Remove( int aStartIndex, int aEndIndex ) logArcIdxRemoval( m_shapes[i].first ); // Only remove the arc on the first index // Ensure that m_shapes has been built correctly. - assert( i > aStartIndex || IsSharedPt( i - 1 ) + assert( i > aStartIndex || ( IsSharedPt( i - 1 ) ? m_shapes[i - 1].second == m_shapes[i].first - : m_shapes[i - 1].first == m_shapes[i].first ); + : m_shapes[i - 1].first == m_shapes[i].first ) ); continue; } } - - alg::run_on_pair( m_shapes[i], logArcIdxRemoval ); + else + { + alg::run_on_pair( m_shapes[i], logArcIdxRemoval ); + } } for( auto arc : extra_arcs ) @@ -885,6 +904,8 @@ void SHAPE_LINE_CHAIN::Remove( int aStartIndex, int aEndIndex ) m_shapes.erase( m_shapes.begin() + aStartIndex, m_shapes.begin() + aEndIndex + 1 ); m_points.erase( m_points.begin() + aStartIndex, m_points.begin() + aEndIndex + 1 ); assert( m_shapes.size() == m_points.size() ); + + SetClosed( closedState ); } @@ -1077,32 +1098,28 @@ void SHAPE_LINE_CHAIN::RemoveShape( int aPointIndex ) if( aPointIndex < 0 ) aPointIndex += PointCount(); + if( aPointIndex >= PointCount() || aPointIndex < 0 ) + return; // Invalid index, fail gracefully + if( m_shapes[aPointIndex] == SHAPES_ARE_PT ) { Remove( aPointIndex ); return; } - //@todo should this be replaced to use NextShape() / PrevShape()? int start = aPointIndex; int end = aPointIndex; int arcIdx = ArcIndex( aPointIndex ); - if( !IsSharedPt( aPointIndex ) ) + if( !IsArcStart( start ) ) { // aPointIndex is not a shared point, so iterate backwards to find the start of the arc - while( start >= 0 && m_shapes[start].first == arcIdx ) - start--; - - // Check if the previous point might be a shared point and decrement 'start' if so - if( start >= 1 && m_shapes[static_cast( start ) - 1].second == arcIdx ) + while( start > 0 && ArcIndex( static_cast( start ) - 1 ) == arcIdx ) start--; } - // For the end point we only need to check the first element in m_shapes (the second one is only - // populated if there is an arc after the current one sharing the same point). - while( end < static_cast( m_shapes.size() ) - 1 && m_shapes[end].first == arcIdx ) - end++; + if( !IsArcEnd( end ) || start == end ) + end = NextShape( end ); // can be -1 to indicate end of chain Remove( start, end ); } diff --git a/qa/tests/libs/kimath/geometry/test_shape_line_chain.cpp b/qa/tests/libs/kimath/geometry/test_shape_line_chain.cpp index dd1794f0de..4a2790eb65 100644 --- a/qa/tests/libs/kimath/geometry/test_shape_line_chain.cpp +++ b/qa/tests/libs/kimath/geometry/test_shape_line_chain.cpp @@ -288,6 +288,126 @@ BOOST_AUTO_TEST_CASE( SimplifyDuplicatePoint ) } +struct REMOVE_SHAPE_CASE +{ + std::string m_ctx_name; + SHAPE_LINE_CHAIN m_chain; + int m_shape_count; + int m_arc_count; + int m_remove_index; + int m_expected_shape_count; + int m_expected_arc_count; +}; + +static const std::vector remove_shape_cases = + { + { "Circle1Arc - 1st arc - index on start", SLC_CASES().Circle1Arc, 1, 1, 0, 0, 0 }, + { "Circle1Arc - 1st arc - index on mid", SLC_CASES().Circle1Arc, 1, 1, 8, 0, 0 }, + { "Circle1Arc - 1st arc - index on end", SLC_CASES().Circle1Arc, 1, 1, 14, 0, 0 }, + { "Circle1Arc - 1st arc - index on -1", SLC_CASES().Circle1Arc, 1, 1, -1, 0, 0 }, + { "Circle1Arc - invalid index", SLC_CASES().Circle1Arc, 1, 1, 15, 1, 1 }, + + { "Circle2Arcs - 1st arc - index on start", SLC_CASES().Circle2Arcs, 2, 2, 0, 2, 1 }, + { "Circle2Arcs - 1st arc - index on mid", SLC_CASES().Circle2Arcs, 2, 2, 3, 2, 1 }, + { "Circle2Arcs - 1st arc - index on end", SLC_CASES().Circle2Arcs, 2, 2, 7, 2, 1 }, + { "Circle2Arcs - 2nd arc - index on start", SLC_CASES().Circle2Arcs, 2, 2, 8, 2, 1 }, + { "Circle2Arcs - 2nd arc - index on mid", SLC_CASES().Circle2Arcs, 2, 2, 11, 2, 1 }, + { "Circle2Arcs - 2nd arc - index on end", SLC_CASES().Circle2Arcs, 2, 2, 15, 2, 1 }, + { "Circle2Arcs - 2nd arc - index on -1", SLC_CASES().Circle2Arcs, 2, 2, -1, 2, 1 }, + { "Circle2Arcs - invalid index", SLC_CASES().Circle2Arcs, 2, 2, 16, 2, 2 }, + + { "ArcsCoinc. - 1st arc - idx on start", SLC_CASES().ArcsCoincident, 2, 2, 0, 1, 1 }, + { "ArcsCoinc. - 1st arc - idx on mid", SLC_CASES().ArcsCoincident, 2, 2, 3, 1, 1 }, + { "ArcsCoinc. - 1st arc - idx on end", SLC_CASES().ArcsCoincident, 2, 2, 7, 1, 1 }, + { "ArcsCoinc. - 2nd arc - idx on start", SLC_CASES().ArcsCoincident, 2, 2, 8, 1, 1 }, + { "ArcsCoinc. - 2nd arc - idx on mid", SLC_CASES().ArcsCoincident, 2, 2, 10, 1, 1 }, + { "ArcsCoinc. - 2nd arc - idx on end", SLC_CASES().ArcsCoincident, 2, 2, 13, 1, 1 }, + { "ArcsCoinc. - 2nd arc - idx on -1", SLC_CASES().ArcsCoincident, 2, 2, -1, 1, 1 }, + { "ArcsCoinc. - invalid idx", SLC_CASES().ArcsCoincident, 2, 2, 14, 2, 2 }, + { "ArcsCoinc. - 1st arc - idx on start", SLC_CASES().ArcsCoincident, 2, 2, 0, 1, 1 }, + + { "A.Co.Closed - 1st arc - idx on start", SLC_CASES().ArcsCoincidentClosed, 3, 2, 1, 2, 1 }, + { "A.Co.Closed - 1st arc - idx on mid", SLC_CASES().ArcsCoincidentClosed, 3, 2, 3, 2, 1 }, + { "A.Co.Closed - 1st arc - idx on end", SLC_CASES().ArcsCoincidentClosed, 3, 2, 7, 2, 1 }, + { "A.Co.Closed - 2nd arc - idx on start", SLC_CASES().ArcsCoincidentClosed, 3, 2, 8, 2, 1 }, + { "A.Co.Closed - 2nd arc - idx on mid", SLC_CASES().ArcsCoincidentClosed, 3, 2, 10, 2, 1 }, + { "A.Co.Closed - 2nd arc - idx on end", SLC_CASES().ArcsCoincidentClosed, 3, 2, 13, 2, 1 }, + { "A.Co.Closed - 2nd arc - idx on -1", SLC_CASES().ArcsCoincidentClosed, 3, 2, -1, 2, 1 }, + { "A.Co.Closed - invalid idx", SLC_CASES().ArcsCoincidentClosed, 3, 2, 14, 3, 2 }, + + { "ArcsIndep. - 1st arc - idx on start", SLC_CASES().ArcsIndependent, 3, 2, 0, 1, 1 }, + { "ArcsIndep. - 1st arc - idx on mid", SLC_CASES().ArcsIndependent, 3, 2, 3, 1, 1 }, + { "ArcsIndep. - 1st arc - idx on end", SLC_CASES().ArcsIndependent, 3, 2, 8, 1, 1 }, + { "ArcsIndep. - 2nd arc - idx on start", SLC_CASES().ArcsIndependent, 3, 2, 9, 1, 1 }, + { "ArcsIndep. - 2nd arc - idx on mid", SLC_CASES().ArcsIndependent, 3, 2, 12, 1, 1 }, + { "ArcsIndep. - 2nd arc - idx on end", SLC_CASES().ArcsIndependent, 3, 2, 17, 1, 1 }, + { "ArcsIndep. - 2nd arc - idx on -1", SLC_CASES().ArcsIndependent, 3, 2, -1, 1, 1 }, + { "ArcsIndep. - invalid idx", SLC_CASES().ArcsIndependent, 3, 2, 18, 3, 2 }, + + { "Dup.Arcs - 1st arc - idx on start", SLC_CASES().DuplicateArcs, 4, 3, 0, 3, 2 }, + { "Dup.Arcs - 1st arc - idx on mid", SLC_CASES().DuplicateArcs, 4, 3, 3, 3, 2 }, + { "Dup.Arcs - 1st arc - idx on end", SLC_CASES().DuplicateArcs, 4, 3, 7, 3, 2 }, + { "Dup.Arcs - 2nd arc - idx on start", SLC_CASES().DuplicateArcs, 4, 3, 8, 3, 2 }, + { "Dup.Arcs - 2nd arc - idx on mid", SLC_CASES().DuplicateArcs, 4, 3, 10, 3, 2 }, + { "Dup.Arcs - 2nd arc - idx on end", SLC_CASES().DuplicateArcs, 4, 3, 13, 3, 2 }, + { "Dup.Arcs - 3rd arc - idx on start", SLC_CASES().DuplicateArcs, 4, 3, 14, 2, 2 }, + { "Dup.Arcs - 3rd arc - idx on mid", SLC_CASES().DuplicateArcs, 4, 3, 17, 2, 2 }, + { "Dup.Arcs - 3rd arc - idx on end", SLC_CASES().DuplicateArcs, 4, 3, 19, 2, 2 }, + { "Dup.Arcs - 3rd arc - idx on -1", SLC_CASES().DuplicateArcs, 4, 3, -1, 2, 2 }, + { "Dup.Arcs - invalid idx", SLC_CASES().DuplicateArcs, 4, 3, 20, 4, 3 }, + + { "Arcs Mixed - 1st arc - idx on start", SLC_CASES().ArcsAndSegMixed, 4, 2, 0, 2, 1 }, + { "Arcs Mixed - 1st arc - idx on mid", SLC_CASES().ArcsAndSegMixed, 4, 2, 3, 2, 1 }, + { "Arcs Mixed - 1st arc - idx on end", SLC_CASES().ArcsAndSegMixed, 4, 2, 8, 2, 1 }, + { "Arcs Mixed - Straight segment", SLC_CASES().ArcsAndSegMixed, 4, 2, 9, 3, 2 }, + { "Arcs Mixed - 2nd arc - idx on start", SLC_CASES().ArcsAndSegMixed, 4, 2, 10, 2, 1 }, + { "Arcs Mixed - 2nd arc - idx on mid", SLC_CASES().ArcsAndSegMixed, 4, 2, 14, 2, 1 }, + { "Arcs Mixed - 2nd arc - idx on end", SLC_CASES().ArcsAndSegMixed, 4, 2, 18, 2, 1 }, + { "Arcs Mixed - 2nd arc - idx on -1", SLC_CASES().ArcsAndSegMixed, 4, 2, -1, 2, 1 }, + { "Arcs Mixed - invalid idx", SLC_CASES().ArcsAndSegMixed, 4, 2, 19, 4, 2 } + }; + + +BOOST_AUTO_TEST_CASE( RemoveShape ) +{ + for( const REMOVE_SHAPE_CASE& c : remove_shape_cases ) + { + BOOST_TEST_CONTEXT( c.m_ctx_name ) + { + SHAPE_LINE_CHAIN slc_case = c.m_chain; // make a copy to edit + BOOST_CHECK_EQUAL( slc_case.ShapeCount(), c.m_shape_count ); + BOOST_CHECK_EQUAL( slc_case.ArcCount(), c.m_arc_count ); + BOOST_CHECK_EQUAL( GEOM_TEST::IsOutlineValid( slc_case ), true ); + slc_case.RemoveShape( c.m_remove_index ); + BOOST_CHECK_EQUAL( slc_case.ShapeCount(), c.m_expected_shape_count ); + BOOST_CHECK_EQUAL( slc_case.ArcCount(), c.m_expected_arc_count ); + BOOST_CHECK_EQUAL( GEOM_TEST::IsOutlineValid( slc_case ), true ); + } + } +} + + +BOOST_AUTO_TEST_CASE( RemoveShapeAfterSimplify ) +{ + for( const REMOVE_SHAPE_CASE& c : remove_shape_cases ) + { + BOOST_TEST_CONTEXT( c.m_ctx_name ) + { + SHAPE_LINE_CHAIN slc_case = c.m_chain; // make a copy to edit + BOOST_CHECK_EQUAL( GEOM_TEST::IsOutlineValid( slc_case ), true ); + BOOST_CHECK_EQUAL( slc_case.ShapeCount(), c.m_shape_count ); + BOOST_CHECK_EQUAL( slc_case.ArcCount(), c.m_arc_count ); + slc_case.Simplify(); + BOOST_CHECK_EQUAL( GEOM_TEST::IsOutlineValid( slc_case ), true ); + BOOST_CHECK_EQUAL( slc_case.ShapeCount(), c.m_shape_count ); + BOOST_CHECK_EQUAL( slc_case.ArcCount(), c.m_arc_count ); + slc_case.RemoveShape( c.m_remove_index ); + BOOST_CHECK_EQUAL( GEOM_TEST::IsOutlineValid( slc_case ), true ); + BOOST_CHECK_EQUAL( slc_case.ShapeCount(), c.m_expected_shape_count ); + BOOST_CHECK_EQUAL( slc_case.ArcCount(), c.m_expected_arc_count ); + } + } +} BOOST_AUTO_TEST_CASE( ShapeCount )