From 17dcfdbf7ab600ab6a15143d5b8ea8a9f598bb66 Mon Sep 17 00:00:00 2001 From: Seth Hillbrand Date: Mon, 13 Jan 2025 16:02:26 -0800 Subject: [PATCH] Fixup multiple issues with DP checks Handle arcs properly even when not exactly concentric. Properly check for interfering elements that would prevent coupling. Avoiding those that are directly connected Fixes https://gitlab.com/kicad/code/kicad/-/issues/17967 (cherry picked from commit 3e8791b88d2f29f3b9379f34c668f5a1545d41ab) --- libs/kimath/include/geometry/shape_arc.h | 11 ++ libs/kimath/src/geometry/shape_arc.cpp | 71 ++++++++++++ .../drc_test_provider_diff_pair_coupling.cpp | 109 +++++++++--------- qa/tests/pcbnew/drc/test_drc_regressions.cpp | 4 +- 4 files changed, 141 insertions(+), 54 deletions(-) diff --git a/libs/kimath/include/geometry/shape_arc.h b/libs/kimath/include/geometry/shape_arc.h index 094dfe932a..89f3ffc698 100644 --- a/libs/kimath/include/geometry/shape_arc.h +++ b/libs/kimath/include/geometry/shape_arc.h @@ -118,6 +118,17 @@ public: VECTOR2I NearestPoint( const VECTOR2I& aP ) const; + + /** + * Compute closest points between this arc and \a aArc. + * + * @param aPtA point on this arc (output) + * @param aPtB point on the other arc (output) + * @param aDistSq squared distance between points (output) + * @return true if the operation was successful + */ + bool NearestPoints( const SHAPE_ARC& aArc, VECTOR2I& aPtA, VECTOR2I& aPtB, int64_t& aDistSq ) const; + bool Collide( const SEG& aSeg, int aClearance = 0, int* aActual = nullptr, VECTOR2I* aLocation = nullptr ) const override; bool Collide( const VECTOR2I& aP, int aClearance = 0, int* aActual = nullptr, diff --git a/libs/kimath/src/geometry/shape_arc.cpp b/libs/kimath/src/geometry/shape_arc.cpp index f033c59900..ed67dc56c8 100644 --- a/libs/kimath/src/geometry/shape_arc.cpp +++ b/libs/kimath/src/geometry/shape_arc.cpp @@ -395,6 +395,77 @@ VECTOR2I SHAPE_ARC::NearestPoint( const VECTOR2I& aP ) const } +bool SHAPE_ARC::NearestPoints( const SHAPE_ARC& aArc, VECTOR2I& aPtA, VECTOR2I& aPtB, + int64_t& aDistSq ) const +{ + aDistSq = std::numeric_limits::max(); + + VECTOR2I center1 = GetCenter(); + VECTOR2I center2 = aArc.GetCenter(); + + int64_t dist = center1.SquaredDistance( center2 ); + + // Start by checking endpoints + std::vector pts1 = { m_start, m_end }; + std::vector pts2 = { aArc.GetP0(), aArc.GetP1() }; + + for( const VECTOR2I& pt1 : pts1 ) + { + for( const VECTOR2I& pt2 : pts2 ) + { + int64_t distSq = pt1.SquaredDistance( pt2 ); + + if( distSq < aDistSq ) + { + aDistSq = distSq; + aPtA = pt1; + aPtB = pt2; + } + } + } + + // If the centers are the same, the end points are always the closest + // or at least equidistant + if( dist > 0 ) + { + CIRCLE circle1( center1, GetRadius() ); + CIRCLE circle2( center2, aArc.GetRadius() ); + + // First check for intersections on the circles + std::vector pts = circle1.Intersect( circle2 ); + + for( const VECTOR2I& pt : pts ) + { + if( sliceContainsPoint( pt ) && aArc.sliceContainsPoint( pt ) ) + { + aPtA = pt; + aPtB = pt; + aDistSq = 0; + return true; + } + } + + // Check for the closest points on the circles + VECTOR2I pt1 = circle1.NearestPoint( center2 ); + VECTOR2I pt2 = circle2.NearestPoint( center1 ); + + if( sliceContainsPoint( pt1 ) && aArc.sliceContainsPoint( pt2 ) ) + { + int64_t distSq = pt1.SquaredDistance( pt2 ); + + if( distSq < aDistSq ) + { + aDistSq = distSq; + aPtA = pt1; + aPtB = pt2; + } + } + } + + return true; +} + + bool SHAPE_ARC::Collide( const VECTOR2I& aP, int aClearance, int* aActual, VECTOR2I* aLocation ) const { diff --git a/pcbnew/drc/drc_test_provider_diff_pair_coupling.cpp b/pcbnew/drc/drc_test_provider_diff_pair_coupling.cpp index 8a65f0d940..a2cd1bc717 100644 --- a/pcbnew/drc/drc_test_provider_diff_pair_coupling.cpp +++ b/pcbnew/drc/drc_test_provider_diff_pair_coupling.cpp @@ -193,6 +193,10 @@ static bool commonParallelProjection( const PCB_ARC& p, const PCB_ARC& n, SHAPE_ pClip = SHAPE_ARC( p_center, p_start_pt, clip_total_angle ); nClip = SHAPE_ARC( n_center, n_start_pt, clip_total_angle ); + if( std::abs( pClip.GetP0().x - pClip.GetArcMid().x ) < 10 + || std::abs( nClip.GetP0().x - nClip.GetArcMid().x ) < 10 ) + return false; + return true; } @@ -240,7 +244,9 @@ struct DIFF_PAIR_COUPLED_SEGMENTS SHAPE_ARC coupledArcP; PCB_TRACK* parentN; PCB_TRACK* parentP; - int computedGap; + int64_t computedGap; + VECTOR2I nearestN; + VECTOR2I nearestP; PCB_LAYER_ID layer; bool couplingFailMin; bool couplingFailMax; @@ -272,8 +278,7 @@ static void extractDiffPairCoupledItems( DIFF_PAIR_ITEMS& aDp ) for( BOARD_CONNECTED_ITEM* itemP : aDp.itemsP ) { PCB_TRACK* sp = dyn_cast( itemP ); - std::optional bestCoupled; - int bestGap = std::numeric_limits::max(); + std::vector> coupled_vec; if( !sp ) continue; @@ -293,7 +298,7 @@ static void extractDiffPairCoupledItems( DIFF_PAIR_ITEMS& aDp ) // Segments that are ~ 1 IU in length per side are approximately parallel (tolerance is 1 IU) // with everything and their parallel projection is < 1 IU, leading to bad distance calculations - if( ssp.SquaredLength() > 2 && ssn.SquaredLength() > 2 && ssp.ApproxParallel(ssn) ) + if( ssp.SquaredLength() > 2 && ssn.SquaredLength() > 2 && !ssp.Intersect( ssn, false, true ) ) { DIFF_PAIR_COUPLED_SEGMENTS cpair; bool coupled = commonParallelProjection( ssp, ssn, cpair.coupledP, cpair.coupledN ); @@ -303,33 +308,33 @@ static void extractDiffPairCoupledItems( DIFF_PAIR_ITEMS& aDp ) cpair.parentP = sp; cpair.parentN = sn; cpair.layer = sp->GetLayer(); - cpair.computedGap = (cpair.coupledP.A - cpair.coupledN.A).EuclideanNorm(); + cpair.coupledP.NearestPoints( cpair.coupledN, cpair.nearestP, cpair.nearestN, + cpair.computedGap ); + cpair.computedGap = std::sqrt( cpair.computedGap ); // NearestPoints returns squared distance cpair.computedGap -= ( sp->GetWidth() + sn->GetWidth() ) / 2; - - if( cpair.computedGap < bestGap ) - { - bestGap = cpair.computedGap; - bestCoupled = cpair; - } + coupled_vec.push_back( cpair ); } } } - if( bestCoupled ) + for( auto coupled : coupled_vec ) { auto excludeSelf = [&]( BOARD_ITEM* aItem ) { - if( aItem == bestCoupled->parentN || aItem == bestCoupled->parentP ) + if( aItem == coupled->parentN || aItem == coupled->parentP ) return false; if( aItem->Type() == PCB_TRACE_T || aItem->Type() == PCB_VIA_T || aItem->Type() == PCB_ARC_T ) { - BOARD_CONNECTED_ITEM* bci = static_cast( aItem ); + PCB_TRACK* bci = static_cast( aItem ); - if( bci->GetNetCode() == bestCoupled->parentN->GetNetCode() - || bci->GetNetCode() == bestCoupled->parentP->GetNetCode() ) + // Directly connected items don't count + if( bci->HitTest( coupled->coupledN.A, 0 ) + || bci->HitTest( coupled->coupledN.B, 0 ) + || bci->HitTest( coupled->coupledP.A, 0 ) + || bci->HitTest( coupled->coupledP.B, 0 ) ) { return false; } @@ -338,17 +343,15 @@ static void extractDiffPairCoupledItems( DIFF_PAIR_ITEMS& aDp ) return true; }; - SHAPE_SEGMENT checkSegStart( bestCoupled->coupledP.A, bestCoupled->coupledN.A ); - SHAPE_SEGMENT checkSegEnd( bestCoupled->coupledP.B, bestCoupled->coupledN.B ); - DRC_RTREE* tree = bestCoupled->parentP->GetBoard()->m_CopperItemRTreeCache.get(); + SHAPE_SEGMENT checkSeg( coupled->nearestN, coupled->nearestP ); + DRC_RTREE* tree = coupled->parentP->GetBoard()->m_CopperItemRTreeCache.get(); // check if there's anything in between the segments suspected to be coupled. If // there's nothing, assume they are really coupled. - if( !tree->CheckColliding( &checkSegStart, sp->GetLayer(), 0, excludeSelf ) - && !tree->CheckColliding( &checkSegEnd, sp->GetLayer(), 0, excludeSelf ) ) + if( !tree->CheckColliding( &checkSeg, sp->GetLayer(), 0, excludeSelf ) ) { - aDp.coupled.push_back( *bestCoupled ); + aDp.coupled.push_back( *coupled ); } } } @@ -356,8 +359,7 @@ static void extractDiffPairCoupledItems( DIFF_PAIR_ITEMS& aDp ) for( BOARD_CONNECTED_ITEM* itemP : aDp.itemsP ) { PCB_ARC* sp = dyn_cast( itemP ); - std::optional bestCoupled; - int bestGap = std::numeric_limits::max(); + std::vector> coupled_vec; if( !sp ) continue; @@ -374,7 +376,9 @@ static void extractDiffPairCoupledItems( DIFF_PAIR_ITEMS& aDp ) // Segments that are ~ 1 IU in length per side are approximately parallel (tolerance is 1 IU) // with everything and their parallel projection is < 1 IU, leading to bad distance calculations - if( sp->GetLength() > 2 && sn->GetLength() > 2 && ( sp->GetCenter() - sn->GetCenter() ).SquaredEuclideanNorm() < 4 ) + int64_t sqWidth = static_cast( sp->GetWidth() ) * sp->GetWidth(); + + if( sp->GetLength() > 2 && sn->GetLength() > 2 && sp->GetCenter().SquaredDistance( sn->GetCenter() ) < sqWidth ) { DIFF_PAIR_COUPLED_SEGMENTS cpair; cpair.isArc = true; @@ -385,53 +389,46 @@ static void extractDiffPairCoupledItems( DIFF_PAIR_ITEMS& aDp ) cpair.parentP = sp; cpair.parentN = sn; cpair.layer = sp->GetLayer(); - cpair.computedGap = KiROUND( std::abs( cpair.coupledArcP.GetRadius() - - cpair.coupledArcN.GetRadius() ) ); + cpair.coupledArcP.NearestPoints( cpair.coupledArcN, cpair.nearestP, cpair.nearestN, + cpair.computedGap ); + cpair.computedGap = std::sqrt( cpair.computedGap ); // NearestPoints returns squared distance cpair.computedGap -= ( sp->GetWidth() + sn->GetWidth() ) / 2; - - if( cpair.computedGap < bestGap ) - { - bestGap = cpair.computedGap; - bestCoupled = cpair; - } + coupled_vec.push_back( cpair ); } - } } - if( bestCoupled ) + for( auto coupled : coupled_vec ) { auto excludeSelf = [&] ( BOARD_ITEM *aItem ) { - if( aItem == bestCoupled->parentN || aItem == bestCoupled->parentP ) - { + if( aItem == coupled->parentN || aItem == coupled->parentP ) return false; - } if( aItem->Type() == PCB_TRACE_T || aItem->Type() == PCB_VIA_T || aItem->Type() == PCB_ARC_T ) { auto bci = static_cast( aItem ); - if( bci->GetNetCode() == bestCoupled->parentN->GetNetCode() - || bci->GetNetCode() == bestCoupled->parentP->GetNetCode() ) + if( bci->GetNetCode() == coupled->parentN->GetNetCode() + || bci->GetNetCode() == coupled->parentP->GetNetCode() ) + { return false; + } } return true; }; - SHAPE_SEGMENT checkSegStart( bestCoupled->coupledP.A, bestCoupled->coupledN.A ); - SHAPE_SEGMENT checkSegEnd( bestCoupled->coupledP.B, bestCoupled->coupledN.B ); - DRC_RTREE* tree = bestCoupled->parentP->GetBoard()->m_CopperItemRTreeCache.get(); + SHAPE_SEGMENT checkArcMid( coupled->coupledArcN.GetArcMid(), coupled->coupledArcP.GetArcMid() ); + DRC_RTREE* tree = coupled->parentP->GetBoard()->m_CopperItemRTreeCache.get(); // check if there's anything in between the segments suspected to be coupled. If // there's nothing, assume they are really coupled. - if( !tree->CheckColliding( &checkSegStart, sp->GetLayer(), 0, excludeSelf ) - && !tree->CheckColliding( &checkSegEnd, sp->GetLayer(), 0, excludeSelf ) ) + if( !tree->CheckColliding( &checkArcMid, sp->GetLayer(), 0, excludeSelf ) ) { - aDp.coupled.push_back( *bestCoupled ); + aDp.coupled.push_back( *coupled ); } } } @@ -536,22 +533,29 @@ bool test::DRC_TEST_PROVIDER_DIFF_PAIR_COUPLING::Run() drc_dbg(10, wxT( " coupled prims : %d\n" ), (int) itemSet.coupled.size() ); + std::set allItems; + for( BOARD_CONNECTED_ITEM* item : itemSet.itemsN ) { if( PCB_TRACK* track = dynamic_cast( item ) ) - itemSet.totalLengthN += track->GetLength(); + { + if( allItems.insert( item ).second) + itemSet.totalLengthN += track->GetLength(); + } } for( BOARD_CONNECTED_ITEM* item : itemSet.itemsP ) { if( PCB_TRACK* track = dynamic_cast( item ) ) - itemSet.totalLengthP += track->GetLength(); + { + if( allItems.insert( item ).second) + itemSet.totalLengthP += track->GetLength(); + } } for( DIFF_PAIR_COUPLED_SEGMENTS& dp : itemSet.coupled ) { - int length = dp.coupledN.Length(); - + int length = dp.isArc ? dp.coupledArcN.GetLength() : dp.coupledN.Length(); wxCHECK2( dp.parentN && dp.parentP, continue ); std::shared_ptr overlay = m_drcEngine->GetDebugOverlay(); @@ -567,7 +571,7 @@ bool test::DRC_TEST_PROVIDER_DIFF_PAIR_COUPLING::Run() overlay->Line( dp.coupledN ); } - drc_dbg( 10, wxT( " len %d gap %d l %d\n" ), + drc_dbg( 10, wxT( " len %d gap %ld l %d\n" ), length, dp.computedGap, dp.parentP->GetLayer() ); @@ -598,8 +602,7 @@ bool test::DRC_TEST_PROVIDER_DIFF_PAIR_COUPLING::Run() MessageTextFromValue( itemSet.totalCoupled ), MessageTextFromValue( totalLen ) ) ); - int totalUncoupled = totalLen - itemSet.totalCoupled; - + int totalUncoupled = totalLen - itemSet.totalCoupled; bool uncoupledViolation = false; if( key.uncoupledConstraint && ( !itemSet.itemsP.empty() || !itemSet.itemsN.empty() ) ) diff --git a/qa/tests/pcbnew/drc/test_drc_regressions.cpp b/qa/tests/pcbnew/drc/test_drc_regressions.cpp index d82f4bd91a..6141ffa1a6 100644 --- a/qa/tests/pcbnew/drc/test_drc_regressions.cpp +++ b/qa/tests/pcbnew/drc/test_drc_regressions.cpp @@ -65,6 +65,7 @@ BOOST_FIXTURE_TEST_CASE( DRCFalsePositiveRegressions, DRC_REGRESSION_TEST_FIXTUR "issue15280", // Very wide spokes mis-counted as being single spoke "issue14008", // Net-tie clearance error "issue19521/issue19521", // oval annular ring size + "issue17967/issue17967", // Arc dp coupling "unconnected-netnames/unconnected-netnames", // Raised false schematic partity error }; @@ -128,7 +129,7 @@ BOOST_FIXTURE_TEST_CASE( DRCFalseNegativeRegressions, DRC_REGRESSION_TEST_FIXTUR { { "issue1358", 2 }, { "issue2512", 5 }, - { "issue2528", 1 }, + { "issue2528", 1 }, { "issue5750", 4 }, // Shorting zone fills pass DRC in some cases { "issue5854", 3 }, { "issue6879", 6 }, @@ -144,6 +145,7 @@ BOOST_FIXTURE_TEST_CASE( DRCFalseNegativeRegressions, DRC_REGRESSION_TEST_FIXTUR { "reverse_via", 3 }, // Via/track ordering { "intersectingzones", 1 }, // zones are too close to each other { "fill_bad", 1 }, // zone max BBox was too small + { "issue18878", 9 }, { "issue19325/issue19325", 4 }, // Overlapping pad annular ring calculation };