From 62d959ed0e39ced60545503996f56f68320d23e9 Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Fri, 13 Oct 2023 12:25:52 +0100 Subject: [PATCH] Don't assume an error location for PAD::GetEffectivePolygon(). While ERROR_INSIDE was good for plotting, 3D generation, etc., it's not good for generating router hulls. Also reverts part of the change to always use polygons for PNS::SOLIDs. A single shape in a SHAPE_COMPOUND will be faster (and more accurate). Fixes https://gitlab.com/kicad/code/kicad/-/issues/14898 --- .../3d_canvas/create_3Dgraphic_brd_items.cpp | 2 +- 3d-viewer/3d_canvas/create_layer_items.cpp | 2 +- libs/kimath/include/geometry/geometry_utils.h | 2 +- pcbnew/board.cpp | 2 +- pcbnew/connectivity/connectivity_data.cpp | 2 +- pcbnew/convert_shape_list_to_polygon.cpp | 3 +- pcbnew/exporters/step/step_pcb_model.cpp | 2 +- pcbnew/pad.cpp | 43 +++++++++++-------- pcbnew/pad.h | 13 +++--- pcbnew/plot_brditems_plotter.cpp | 2 +- pcbnew/router/pns_kicad_iface.cpp | 25 ++++++----- pcbnew/router/pns_solid.cpp | 2 - pcbnew/router/pns_solid.h | 9 +--- pcbnew/router/pns_topology.cpp | 4 +- pcbnew/tools/pcb_grid_helper.cpp | 2 +- pcbnew/tracks_cleaner.cpp | 3 +- pcbnew/zone_filler.cpp | 4 +- 17 files changed, 62 insertions(+), 60 deletions(-) diff --git a/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp b/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp index 34b4271593..a4cf74ddcb 100644 --- a/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp +++ b/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp @@ -737,7 +737,7 @@ void BOARD_ADAPTER::buildPadOutlineAsSegments( const PAD* aPad, CONTAINER_2D_BAS else { // For other shapes, add outlines as thick segments in polygon buffer - const std::shared_ptr& corners = aPad->GetEffectivePolygon(); + const std::shared_ptr& corners = aPad->GetEffectivePolygon( ERROR_INSIDE ); const SHAPE_LINE_CHAIN& path = corners->COutline( 0 ); for( int j = 0; j < path.PointCount(); j++ ) diff --git a/3d-viewer/3d_canvas/create_layer_items.cpp b/3d-viewer/3d_canvas/create_layer_items.cpp index 847689e8b2..4e0cfcb4f6 100644 --- a/3d-viewer/3d_canvas/create_layer_items.cpp +++ b/3d-viewer/3d_canvas/create_layer_items.cpp @@ -72,7 +72,7 @@ void buildPadOutlineAsPolygon( const PAD* aPad, SHAPE_POLY_SET& aBuffer, int aWi else { // For other shapes, add outlines as thick segments in polygon buffer - const SHAPE_LINE_CHAIN& path = aPad->GetEffectivePolygon()->COutline( 0 ); + const SHAPE_LINE_CHAIN& path = aPad->GetEffectivePolygon( ERROR_INSIDE )->COutline( 0 ); for( int ii = 0; ii < path.PointCount(); ++ii ) { diff --git a/libs/kimath/include/geometry/geometry_utils.h b/libs/kimath/include/geometry/geometry_utils.h index ec6296fefa..b2ef64cddc 100644 --- a/libs/kimath/include/geometry/geometry_utils.h +++ b/libs/kimath/include/geometry/geometry_utils.h @@ -40,7 +40,7 @@ * or inside of the curve? (Generally speaking filled shape errors go on the inside * and knockout errors go on the outside. This preserves minimum clearances.) */ -enum ERROR_LOC { ERROR_OUTSIDE, ERROR_INSIDE }; +enum ERROR_LOC { ERROR_OUTSIDE = 0, ERROR_INSIDE }; /** * @return the number of segments to approximate a arc by segments diff --git a/pcbnew/board.cpp b/pcbnew/board.cpp index 0c8818eab3..438c2e137f 100644 --- a/pcbnew/board.cpp +++ b/pcbnew/board.cpp @@ -1975,7 +1975,7 @@ std::tuple BOARD::GetTrackLength( const PCB_TRACK& aTrack ) VECTOR2I loc; // We may not collide even if we passed the bounding-box hit test - if( pad->GetEffectivePolygon()->Collide( trackSeg, 0, nullptr, &loc ) ) + if( pad->GetEffectivePolygon( ERROR_INSIDE )->Collide( trackSeg, 0, nullptr, &loc ) ) { // Part 1: length of the seg to the intersection with the pad poly if( hitStart ) diff --git a/pcbnew/connectivity/connectivity_data.cpp b/pcbnew/connectivity/connectivity_data.cpp index e9825cf6b7..3a61e5f1d9 100644 --- a/pcbnew/connectivity/connectivity_data.cpp +++ b/pcbnew/connectivity/connectivity_data.cpp @@ -448,7 +448,7 @@ bool CONNECTIVITY_DATA::IsConnectedOnLayer( const BOARD_CONNECTED_ITEM *aItem, i if( zone->IsFilled() ) { const SHAPE_POLY_SET* zoneFill = zone->GetFill( ToLAYER_ID( aLayer ) ); - const SHAPE_LINE_CHAIN& padHull = pad->GetEffectivePolygon()->Outline( 0 ); + const SHAPE_LINE_CHAIN& padHull = pad->GetEffectivePolygon( ERROR_INSIDE )->Outline( 0 ); for( const VECTOR2I& pt : zoneFill->COutline( islandIdx ).CPoints() ) { diff --git a/pcbnew/convert_shape_list_to_polygon.cpp b/pcbnew/convert_shape_list_to_polygon.cpp index 2bd9e303f9..b99964854e 100644 --- a/pcbnew/convert_shape_list_to_polygon.cpp +++ b/pcbnew/convert_shape_list_to_polygon.cpp @@ -142,7 +142,8 @@ static bool isCopperOutside( const FOOTPRINT* aFootprint, SHAPE_POLY_SET& aShape { SHAPE_POLY_SET poly = aShape.CloneDropTriangulation(); - poly.BooleanIntersection( *pad->GetEffectivePolygon(), SHAPE_POLY_SET::PM_FAST ); + poly.BooleanIntersection( *pad->GetEffectivePolygon( ERROR_INSIDE ), + SHAPE_POLY_SET::PM_FAST ); if( poly.OutlineCount() == 0 ) { diff --git a/pcbnew/exporters/step/step_pcb_model.cpp b/pcbnew/exporters/step/step_pcb_model.cpp index 38395206dc..3eae1def65 100644 --- a/pcbnew/exporters/step/step_pcb_model.cpp +++ b/pcbnew/exporters/step/step_pcb_model.cpp @@ -215,7 +215,7 @@ STEP_PCB_MODEL::~STEP_PCB_MODEL() bool STEP_PCB_MODEL::AddPadShape( const PAD* aPad, const VECTOR2D& aOrigin ) { - const std::shared_ptr& pad_shape = aPad->GetEffectivePolygon(); + const std::shared_ptr& pad_shape = aPad->GetEffectivePolygon( ERROR_INSIDE ); bool success = true; VECTOR2I pos = aPad->GetPosition(); diff --git a/pcbnew/pad.cpp b/pcbnew/pad.cpp index fda4d19cf0..5087f0d921 100644 --- a/pcbnew/pad.cpp +++ b/pcbnew/pad.cpp @@ -356,12 +356,12 @@ void PAD::SetChamferRectRatio( double aChamferScale ) } -const std::shared_ptr& PAD::GetEffectivePolygon() const +const std::shared_ptr& PAD::GetEffectivePolygon( ERROR_LOC aErrorLoc ) const { - if( m_polyDirty ) - BuildEffectivePolygon(); + if( m_polyDirty[ aErrorLoc ] ) + BuildEffectivePolygon( aErrorLoc ); - return m_effectivePolygon; + return m_effectivePolygon[ aErrorLoc ]; } @@ -414,7 +414,7 @@ std::shared_ptr PAD::GetEffectiveHoleShape() const int PAD::GetBoundingRadius() const { if( m_polyDirty ) - BuildEffectivePolygon(); + BuildEffectivePolygon( ERROR_OUTSIDE ); return m_effectiveBoundingRadius; } @@ -595,41 +595,46 @@ void PAD::BuildEffectiveShapes( PCB_LAYER_ID aLayer ) const } -void PAD::BuildEffectivePolygon() const +void PAD::BuildEffectivePolygon( ERROR_LOC aErrorLoc ) const { std::lock_guard RAII_lock( m_polyBuildingLock ); // If we had to wait for the lock then we were probably waiting for someone else to // finish rebuilding the shapes. So check to see if they're clean now. - if( !m_polyDirty ) + if( !m_polyDirty[ aErrorLoc ] ) return; const BOARD* board = GetBoard(); int maxError = board ? board->GetDesignSettings().m_MaxError : ARC_HIGH_DEF; // Polygon - m_effectivePolygon = std::make_shared(); - TransformShapeToPolygon( *m_effectivePolygon, UNDEFINED_LAYER, 0, maxError, ERROR_INSIDE ); + std::shared_ptr& effectivePolygon = m_effectivePolygon[ aErrorLoc ]; + + effectivePolygon = std::make_shared(); + TransformShapeToPolygon( *effectivePolygon, UNDEFINED_LAYER, 0, maxError, aErrorLoc ); // Bounding radius // // PADSTACKS TODO: these will both need to cycle through all layers to get the largest // values.... - m_effectiveBoundingRadius = 0; - - for( int cnt = 0; cnt < m_effectivePolygon->OutlineCount(); ++cnt ) + if( aErrorLoc == ERROR_OUTSIDE ) { - const SHAPE_LINE_CHAIN& poly = m_effectivePolygon->COutline( cnt ); + m_effectiveBoundingRadius = 0; - for( int ii = 0; ii < poly.PointCount(); ++ii ) + for( int cnt = 0; cnt < effectivePolygon->OutlineCount(); ++cnt ) { - int dist = KiROUND( ( poly.CPoint( ii ) - m_pos ).EuclideanNorm() ); - m_effectiveBoundingRadius = std::max( m_effectiveBoundingRadius, dist ); + const SHAPE_LINE_CHAIN& poly = effectivePolygon->COutline( cnt ); + + for( int ii = 0; ii < poly.PointCount(); ++ii ) + { + int dist = KiROUND( ( poly.CPoint( ii ) - m_pos ).EuclideanNorm() ); + m_effectiveBoundingRadius = std::max( m_effectiveBoundingRadius, dist ); + } } } // All done - m_polyDirty = false; + m_polyDirty[ aErrorLoc ] = false; } @@ -1086,7 +1091,7 @@ bool PAD::HitTest( const VECTOR2I& aPosition, int aAccuracy ) const if( delta.SquaredEuclideanNorm() > SEG::Square( boundingRadius ) ) return false; - return GetEffectivePolygon()->Contains( aPosition, -1, aAccuracy ); + return GetEffectivePolygon( ERROR_INSIDE )->Contains( aPosition, -1, aAccuracy ); } @@ -1109,7 +1114,7 @@ bool PAD::HitTest( const BOX2I& aRect, bool aContained, int aAccuracy ) const if( !arect.Intersects( bbox ) ) return false; - const std::shared_ptr& poly = GetEffectivePolygon(); + const std::shared_ptr& poly = GetEffectivePolygon( ERROR_INSIDE ); int count = poly->TotalVertices(); diff --git a/pcbnew/pad.h b/pcbnew/pad.h index ff02a99f39..11c197414b 100644 --- a/pcbnew/pad.h +++ b/pcbnew/pad.h @@ -356,13 +356,14 @@ public: bool IsDirty() const { - return m_shapesDirty || m_polyDirty; + return m_shapesDirty || m_polyDirty[ERROR_INSIDE] || m_polyDirty[ERROR_OUTSIDE]; } void SetDirty() { m_shapesDirty = true; - m_polyDirty = true; + m_polyDirty[ERROR_INSIDE] = true; + m_polyDirty[ERROR_OUTSIDE] = true; } void SetLayerSet( LSET aLayers ) override { m_layerMask = aLayers; } @@ -443,7 +444,7 @@ public: GetEffectiveShape( PCB_LAYER_ID aLayer = UNDEFINED_LAYER, FLASHING flashPTHPads = FLASHING::DEFAULT ) const override; - const std::shared_ptr& GetEffectivePolygon() const; + const std::shared_ptr& GetEffectivePolygon( ERROR_LOC aErrorLoc ) const; /** * Return a SHAPE_SEGMENT object representing the pad's hole. @@ -695,7 +696,7 @@ public: * the dirty bit. */ void BuildEffectiveShapes( PCB_LAYER_ID aLayer ) const; - void BuildEffectivePolygon() const; + void BuildEffectivePolygon( ERROR_LOC aErrorLoc ) const; virtual void ViewGetLayers( int aLayers[], int& aCount ) const override; @@ -754,9 +755,9 @@ private: mutable std::shared_ptr m_effectiveShape; mutable std::shared_ptr m_effectiveHoleShape; - mutable bool m_polyDirty; + mutable bool m_polyDirty[2]; mutable std::mutex m_polyBuildingLock; - mutable std::shared_ptr m_effectivePolygon; + mutable std::shared_ptr m_effectivePolygon[2]; mutable int m_effectiveBoundingRadius; int m_subRatsnest; // Variable used to handle subnet (block) number in diff --git a/pcbnew/plot_brditems_plotter.cpp b/pcbnew/plot_brditems_plotter.cpp index 2046f40513..fb6cb425ee 100644 --- a/pcbnew/plot_brditems_plotter.cpp +++ b/pcbnew/plot_brditems_plotter.cpp @@ -260,7 +260,7 @@ void BRDITEMS_PLOTTER::PlotPad( const PAD* aPad, const COLOR4D& aColor, OUTLINE_ default: case PAD_SHAPE::CUSTOM: { - const std::shared_ptr& polygons = aPad->GetEffectivePolygon(); + const std::shared_ptr& polygons = aPad->GetEffectivePolygon( ERROR_INSIDE ); if( polygons->OutlineCount() ) { diff --git a/pcbnew/router/pns_kicad_iface.cpp b/pcbnew/router/pns_kicad_iface.cpp index d6bb18b634..7ca2d22388 100644 --- a/pcbnew/router/pns_kicad_iface.cpp +++ b/pcbnew/router/pns_kicad_iface.cpp @@ -1185,23 +1185,26 @@ std::unique_ptr PNS_KICAD_IFACE_BASE::syncPad( PAD* aPad ) if( aPad->GetDrillSize().x > 0 ) solid->SetHole( new PNS::HOLE( aPad->GetEffectiveHoleShape()->Clone() ) ); - std::shared_ptr shape = aPad->GetEffectiveShape( UNDEFINED_LAYER, - FLASHING::ALWAYS_FLASHED ); - std::shared_ptr polygon = aPad->GetEffectivePolygon(); + // We generate a single SOLID for a pad, so we have to treat it as ALWAYS_FLASHED and then + // perform layer-specific flashing tests internally. + const std::shared_ptr& shape = aPad->GetEffectiveShape( UNDEFINED_LAYER, + FLASHING::ALWAYS_FLASHED ); - if( shape->HasIndexableSubshapes() && polygon->OutlineCount() ) + if( shape->HasIndexableSubshapes() && shape->GetIndexableSubshapeCount() == 1 ) { - solid->SetShape( new SHAPE_SIMPLE( polygon->Outline( 0 ) ) ); + std::vector subshapes; + shape->GetIndexableSubshapes( subshapes ); - // GetEffectivePolygon may produce an approximation of the shape, so we need to account for - // this when building hulls around this shape. - solid->SetExtraClearance( m_board->GetDesignSettings().m_MaxError ); + solid->SetShape( subshapes[0]->Clone() ); } + // For anything that's not a single shape we use a polygon. Multiple shapes have a tendency + // to confuse the hull generator. https://gitlab.com/kicad/code/kicad/-/issues/15553 else { - // Prefer using the original shape if it's not a compound shape; the hulls for - // circular and rectangular pads can be exact. - solid->SetShape( shape->Clone() ); + const std::shared_ptr& poly = aPad->GetEffectivePolygon( ERROR_OUTSIDE ); + + if( poly->OutlineCount() ) + solid->SetShape( new SHAPE_SIMPLE( poly->Outline( 0 ) ) ); } return solid; diff --git a/pcbnew/router/pns_solid.cpp b/pcbnew/router/pns_solid.cpp index 5901bc8ce5..e9fba10479 100644 --- a/pcbnew/router/pns_solid.cpp +++ b/pcbnew/router/pns_solid.cpp @@ -41,8 +41,6 @@ const SHAPE_LINE_CHAIN SOLID::Hull( int aClearance, int aWalkaroundThickness, in if( !m_shape ) return SHAPE_LINE_CHAIN(); - aClearance += ExtraClearance(); - if( m_shape->Type() == SH_COMPOUND ) { SHAPE_COMPOUND* cmpnd = static_cast( m_shape ); diff --git a/pcbnew/router/pns_solid.h b/pcbnew/router/pns_solid.h index 3d4a24a58e..988c08387f 100644 --- a/pcbnew/router/pns_solid.h +++ b/pcbnew/router/pns_solid.h @@ -38,8 +38,7 @@ public: SOLID() : ITEM( SOLID_T ), m_shape( nullptr ), - m_hole( nullptr ), - m_extraClearance( 0 ) + m_hole( nullptr ) { m_movable = false; m_padToDie = 0; @@ -66,7 +65,6 @@ public: m_padToDie = aSolid.m_padToDie; m_orientation = aSolid.m_orientation; m_anchorPoints = aSolid.m_anchorPoints; - m_extraClearance = aSolid.m_extraClearance; } SOLID& operator=( const SOLID& aB ) @@ -81,7 +79,6 @@ public: m_padToDie = aB.m_padToDie; m_orientation = aB.m_orientation; m_anchorPoints = aB.m_anchorPoints; - m_extraClearance = aB.m_extraClearance; return *this; } @@ -138,9 +135,6 @@ public: virtual bool HasHole() const override { return m_hole != nullptr; } virtual HOLE *Hole() const override { return m_hole; } - int ExtraClearance() const { return m_extraClearance; } - void SetExtraClearance( int aClearance ) { m_extraClearance = aClearance; } - private: VECTOR2I m_pos; SHAPE* m_shape; @@ -149,7 +143,6 @@ private: EDA_ANGLE m_orientation; HOLE* m_hole; std::vector m_anchorPoints; - int m_extraClearance; }; } diff --git a/pcbnew/router/pns_topology.cpp b/pcbnew/router/pns_topology.cpp index b16c8b8517..2e0afead53 100644 --- a/pcbnew/router/pns_topology.cpp +++ b/pcbnew/router/pns_topology.cpp @@ -367,7 +367,7 @@ const ITEM_SET TOPOLOGY::AssembleTuningPath( ITEM* aStart, SOLID** aStartPad, SO auto clipLineToPad = []( SHAPE_LINE_CHAIN& aLine, PAD* aPad, bool aForward = true ) { - const std::shared_ptr& shape = aPad->GetEffectivePolygon(); + const auto& shape = aPad->GetEffectivePolygon( ERROR_INSIDE ); int start = aForward ? 0 : aLine.PointCount() - 1; int delta = aForward ? 1 : -1; @@ -413,7 +413,7 @@ const ITEM_SET TOPOLOGY::AssembleTuningPath( ITEM* aStart, SOLID** aStartPad, SO auto processPad = [&]( const JOINT* aJoint, PAD* aPad ) { - const std::shared_ptr& shape = aPad->GetEffectivePolygon(); + const auto& shape = aPad->GetEffectivePolygon( ERROR_INSIDE ); for( int idx = 0; idx < initialPath.Size(); idx++ ) { diff --git a/pcbnew/tools/pcb_grid_helper.cpp b/pcbnew/tools/pcb_grid_helper.cpp index 60a497ea3f..02dbf668a8 100644 --- a/pcbnew/tools/pcb_grid_helper.cpp +++ b/pcbnew/tools/pcb_grid_helper.cpp @@ -642,7 +642,7 @@ void PCB_GRID_HELPER::computeAnchors( BOARD_ITEM* aItem, const VECTOR2I& aRefPos default: { - const std::shared_ptr& outline = aPad->GetEffectivePolygon(); + const auto& outline = aPad->GetEffectivePolygon( ERROR_INSIDE ); if( !outline->IsEmpty() ) { diff --git a/pcbnew/tracks_cleaner.cpp b/pcbnew/tracks_cleaner.cpp index 762912f6a4..d527649a72 100644 --- a/pcbnew/tracks_cleaner.cpp +++ b/pcbnew/tracks_cleaner.cpp @@ -328,7 +328,8 @@ void TRACKS_CLEANER::deleteTracksInPads() track->TransformShapeToPolygon( poly, track->GetLayer(), 0, ARC_HIGH_DEF, ERROR_INSIDE ); - poly.BooleanSubtract( *pad->GetEffectivePolygon(), SHAPE_POLY_SET::PM_FAST ); + poly.BooleanSubtract( *pad->GetEffectivePolygon( ERROR_INSIDE ), + SHAPE_POLY_SET::PM_FAST ); if( poly.IsEmpty() ) { diff --git a/pcbnew/zone_filler.cpp b/pcbnew/zone_filler.cpp index e58dfecd2f..df3486c98a 100644 --- a/pcbnew/zone_filler.cpp +++ b/pcbnew/zone_filler.cpp @@ -125,7 +125,7 @@ bool ZONE_FILLER::Fill( std::vector& aZones, bool aCheck, wxWindow* aPare if( pad->IsDirty() ) { pad->BuildEffectiveShapes( UNDEFINED_LAYER ); - pad->BuildEffectivePolygon(); + pad->BuildEffectivePolygon( ERROR_OUTSIDE ); } } @@ -1864,7 +1864,7 @@ void ZONE_FILLER::buildThermalSpokes( const ZONE* aZone, PCB_LAYER_ID aLayer, seg.B += pad->ShapePos(); // Make sure seg.A is the origin - if( !pad->GetEffectivePolygon()->Contains( seg.A ) ) + if( !pad->GetEffectivePolygon( ERROR_OUTSIDE )->Contains( seg.A ) ) seg.Reverse(); // Trim seg.B to the thermal outline