From d5e129a3a06364842ca2c2eb2d48dfd04ae5f562 Mon Sep 17 00:00:00 2001 From: John Beard Date: Sat, 7 Mar 2026 20:57:28 +0800 Subject: [PATCH] Pcbnew/Allegro: Break out zone merge function. This is a tricky function that really needs a test around it. Also it has several assumptions and traps: - It will not work if the zones have multiple polygons in the poly sets - It will not work if any polygon has line chains in a different order - It is quadratic in number of zones and the compare itself is superlinear (Simplify is called, evilly in a const function, which is quadratic) and in a cyclic comparison, sort, which is presumably nlog-n ish. However, at least for the quadratic nature, profiling shows this to be absolutely insignificant even for the VCU118 board. --- common/CMakeLists.txt | 3 +- libs/kimath/include/geometry/shape_utils.h | 9 +- libs/kimath/src/geometry/shape_utils.cpp | 14 ++ pcbnew/pcb_io/allegro/allegro_builder.cpp | 108 +------------ pcbnew/zone_utils.cpp | 148 ++++++++++++++++++ pcbnew/zone_utils.h | 39 +++++ .../kimath/geometry/test_shape_line_chain.cpp | 25 +++ qa/tests/pcbnew/test_zone.cpp | 103 ++++++++++++ 8 files changed, 342 insertions(+), 107 deletions(-) create mode 100644 pcbnew/zone_utils.cpp create mode 100644 pcbnew/zone_utils.h diff --git a/common/CMakeLists.txt b/common/CMakeLists.txt index 89936ddf5a..cbdba26a39 100644 --- a/common/CMakeLists.txt +++ b/common/CMakeLists.txt @@ -840,7 +840,7 @@ if( KICAD_USE_PCH ) - + ) endif() @@ -949,6 +949,7 @@ set( PCB_COMMON_SRCS ${CMAKE_SOURCE_DIR}/pcbnew/pcb_track.cpp ${CMAKE_SOURCE_DIR}/pcbnew/pcb_generator.cpp ${CMAKE_SOURCE_DIR}/pcbnew/zone.cpp + ${CMAKE_SOURCE_DIR}/pcbnew/zone_utils.cpp ${CMAKE_SOURCE_DIR}/pcbnew/collectors.cpp ${CMAKE_SOURCE_DIR}/pcbnew/connectivity/connectivity_algo.cpp ${CMAKE_SOURCE_DIR}/pcbnew/connectivity/connectivity_items.cpp diff --git a/libs/kimath/include/geometry/shape_utils.h b/libs/kimath/include/geometry/shape_utils.h index 67ad1c023c..3b503d39c4 100644 --- a/libs/kimath/include/geometry/shape_utils.h +++ b/libs/kimath/include/geometry/shape_utils.h @@ -81,6 +81,13 @@ std::array BoxToSegs( const BOX2I& aBox ); */ void CollectBoxCorners( const BOX2I& aBox, std::vector& aCorners ); +/* + * Get a SHAPE_LINE_CHAIN representing the outline of a box. + * + * The first point and winding direction are not specified. + */ +SHAPE_LINE_CHAIN BoxToLineChain( const BOX2I& aBox ); + /** * Get the segments of a box that are in the given direction. * @@ -209,4 +216,4 @@ std::vector MakeRegularPolygonPoints( const VECTOR2I& aCenter, size_t std::vector MakeCrossSegments( const VECTOR2I& aCenter, const VECTOR2I& aSize, EDA_ANGLE aAngle ); -} // namespace KIGEOM \ No newline at end of file +} // namespace KIGEOM diff --git a/libs/kimath/src/geometry/shape_utils.cpp b/libs/kimath/src/geometry/shape_utils.cpp index 4102bb690a..d44b9e08cb 100644 --- a/libs/kimath/src/geometry/shape_utils.cpp +++ b/libs/kimath/src/geometry/shape_utils.cpp @@ -90,6 +90,20 @@ void KIGEOM::CollectBoxCorners( const BOX2I& aBox, std::vector& aCorne } +SHAPE_LINE_CHAIN KIGEOM::BoxToLineChain( const BOX2I& aBox ) +{ + SHAPE_LINE_CHAIN result; + + result.Append( VECTOR2I{ aBox.GetLeft(), aBox.GetTop() } ); + result.Append( VECTOR2I{ aBox.GetRight(), aBox.GetTop() } ); + result.Append( VECTOR2I{ aBox.GetRight(), aBox.GetBottom() } ); + result.Append( VECTOR2I{ aBox.GetLeft(), aBox.GetBottom() } ); + result.SetClosed( true ); + + return result; +} + + std::vector KIGEOM::GetSegsInDirection( const BOX2I& aBox, DIRECTION_45::Directions aDir ) { // clang-format off diff --git a/pcbnew/pcb_io/allegro/allegro_builder.cpp b/pcbnew/pcb_io/allegro/allegro_builder.cpp index 7cc66b9d02..cc1e6d0be1 100644 --- a/pcbnew/pcb_io/allegro/allegro_builder.cpp +++ b/pcbnew/pcb_io/allegro/allegro_builder.cpp @@ -39,6 +39,7 @@ #include #include +#include #include #include #include @@ -49,7 +50,7 @@ #include #include #include -#include +#include using namespace ALLEGRO; @@ -4057,109 +4058,6 @@ void BOARD_BUILDER::createBoardText() } -/** - * Merges zones with identical outlines snd nets on different layers into single multi-layer zones. - * - * The merged zones (and any non-identical zones) are returned, the zones merged from are destroyed, - */ -static std::vector> MergeZones( std::vector> aZones, const BOARD& aBoard ) -{ - std::vector> deduplicatedZones; - size_t mergedCount = 0; - size_t originalCount = aZones.size(); - std::vector merged( aZones.size(), false ); - - for( size_t i = 0; i < aZones.size(); i++ ) - { - if( merged[i] ) - continue; - - ZONE* primary = aZones[i].get(); - const SHAPE_POLY_SET::POLYGON& primaryPolygon = primary->Outline()->CPolygon( 0 ); - LSET layers = primary->GetLayerSet(); - std::unordered_map mergedFills; - - for( size_t j = i + 1; j < aZones.size(); j++ ) - { - if( merged[j] ) - continue; - - ZONE* candidate = aZones[j].get(); - - if( candidate->GetNetCode() != primary->GetNetCode() ) - continue; - - const SHAPE_POLY_SET::POLYGON& candidatePolygon = candidate->Outline()->CPolygon( 0 ); - - if( primaryPolygon.size() != candidatePolygon.size() ) - continue; - - bool polygonsDiffer = false; - - for( size_t lineChainId = 0; lineChainId < primaryPolygon.size(); lineChainId++ ) - { - const SHAPE_LINE_CHAIN& primaryChain = primaryPolygon[lineChainId]; - const SHAPE_LINE_CHAIN& candidateChain = candidatePolygon[lineChainId]; - - if( primaryChain.PointCount() != candidateChain.PointCount() - || primaryChain.BBox() != candidateChain.BBox() - || !primaryChain.CompareGeometry( candidateChain ) ) - { - polygonsDiffer = true; - break; - } - } - - if( !polygonsDiffer ) - { - for( PCB_LAYER_ID layer : candidate->GetLayerSet() ) - { - if( SHAPE_POLY_SET* fill = candidate->GetFill( layer ) ) - mergedFills[layer] = *fill; - } - - layers |= candidate->GetLayerSet(); - merged[j] = true; - mergedCount++; - - wxLogTrace( traceAllegroBuilder, " Merging zone on %s into zone on %s (net %d)", - aBoard.GetLayerName( candidate->GetFirstLayer() ), - aBoard.GetLayerName( primary->GetFirstLayer() ), primary->GetNetCode() ); - } - } - - if( layers != primary->GetLayerSet() ) - { - for( PCB_LAYER_ID layer : primary->GetLayerSet() ) - { - if( SHAPE_POLY_SET* fill = primary->GetFill( layer ) ) - mergedFills[layer] = *fill; - } - - primary->SetLayerSet( layers ); - - for( const auto& [layer, fill] : mergedFills ) - primary->SetFilledPolysList( layer, fill ); - - primary->SetNeedRefill( false ); - primary->SetIsFilled( true ); - } - - // Keep this zone - deduplicatedZones.push_back( std::move( aZones[i] ) ); - } - - if( mergedCount > 0 ) - { - wxLogTrace( traceAllegroBuilder, - " Merged %zu zones into multi-layer zones (%zu zones remain from %zu)", - mergedCount, originalCount - mergedCount, originalCount ); - } - - return deduplicatedZones; -} - - template T> void BulkAddToBoard( BOARD& aBoard, std::vector>&& aItems ) { @@ -4262,7 +4160,7 @@ void BOARD_BUILDER::createZones() // Allegro often defines the same zone outline on multiple copper layers (e.g. // a ground pour spanning all layers). KiCad represents this as a single zone // with multiple fill layers. - std::vector> mergedZones = MergeZones( std::move( boundaryZones ), m_board ); + std::vector> mergedZones = MergeZonesWithSameOutline( std::move( boundaryZones ) ); int mergedCount = mergedZones.size(); BulkAddToBoard( m_board, std::move( mergedZones ) ); diff --git a/pcbnew/zone_utils.cpp b/pcbnew/zone_utils.cpp new file mode 100644 index 0000000000..90f03f33b7 --- /dev/null +++ b/pcbnew/zone_utils.cpp @@ -0,0 +1,148 @@ +/* + * This program source code file is part of KiCad, a free EDA CAD application. + * + * Copyright The KiCad Developers, see AUTHORS.txt for contributors. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 3 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, you may find one here: + * http://www.gnu.org/licenses/gpl-3.0.html + * or you may search the http://www.gnu.org website for the version 3 license, + * or you may write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#include "zone_utils.h" + +#include +#include + + +std::vector> MergeZonesWithSameOutline( std::vector>&& aZones ) +{ + const auto polygonsAreMergeable = []( const SHAPE_POLY_SET::POLYGON& a, const SHAPE_POLY_SET::POLYGON& b ) -> bool + { + if( a.size() != b.size() ) + return false; + + // NOTE: this assumes the polygons have their line chains in the same order + // But that is not actually required for same geometry (i.e. mergeability) + for( size_t lineChainId = 0; lineChainId < a.size(); lineChainId++ ) + { + const SHAPE_LINE_CHAIN& chainA = a[lineChainId]; + const SHAPE_LINE_CHAIN& chainB = b[lineChainId]; + + // Note: this assumes the polygons are either already simplified or that it's + // OK to not merge even if they would be the same after simplification. + if( chainA.PointCount() != chainB.PointCount() || chainA.BBox() != chainB.BBox() + || !chainA.CompareGeometry( chainB ) ) + { + // Different geometry, can't merge + return false; + } + } + + return true; + }; + + const auto zonesAreMergeable = [&]( const ZONE& a, const ZONE& b ) -> bool + { + if( a.GetNetCode() != b.GetNetCode() ) + return false; + + // Can't merge rule areas with zone fills + if( a.GetIsRuleArea() != b.GetIsRuleArea() ) + return false; + + const SHAPE_POLY_SET* polySetA = a.Outline(); + const SHAPE_POLY_SET* polySetB = b.Outline(); + + if( polySetA->OutlineCount() != polySetB->OutlineCount() ) + return false; + + if( polySetA->OutlineCount() == 0 ) + { + // both have no outline, so they are the same, but we must not + // derefence them, as they are empty + return true; + } + + // REVIEW: this assumes the zones only have a single polygon in the + const SHAPE_POLY_SET::POLYGON& polyA = polySetA->CPolygon( 0 ); + const SHAPE_POLY_SET::POLYGON& polyB = polySetB->CPolygon( 0 ); + + return polygonsAreMergeable( polyA, polyB ); + }; + + std::vector> deduplicatedZones; + size_t mergedCount = 0; + // Map of zone indexes that we have already merged into a prior zone + std::vector merged( aZones.size(), false ); + + for( size_t i = 0; i < aZones.size(); i++ ) + { + // This one has already been subsumed into a prior zone, so skip it + // and it will be dropped at the end. + if( merged[i] ) + continue; + + ZONE& primary = *aZones[i]; + LSET layers = primary.GetLayerSet(); + std::unordered_map mergedFills; + + for( size_t j = i + 1; j < aZones.size(); j++ ) + { + // This zone has already been subsumed by a prior zone, so it + // cannot be merged into another primary + if( merged[j] ) + continue; + + ZONE& candidate = *aZones[j]; + bool canMerge = zonesAreMergeable( primary, candidate ); + + if( canMerge ) + { + for( PCB_LAYER_ID layer : candidate.GetLayerSet() ) + { + if( SHAPE_POLY_SET* fill = candidate.GetFill( layer ) ) + mergedFills[layer] = *fill; + } + + layers |= candidate.GetLayerSet(); + merged[j] = true; + mergedCount++; + } + } + + if( layers != primary.GetLayerSet() ) + { + for( PCB_LAYER_ID layer : primary.GetLayerSet() ) + { + if( SHAPE_POLY_SET* fill = primary.GetFill( layer ) ) + mergedFills[layer] = *fill; + } + + primary.SetLayerSet( layers ); + + for( const auto& [layer, fill] : mergedFills ) + primary.SetFilledPolysList( layer, fill ); + + primary.SetNeedRefill( false ); + primary.SetIsFilled( true ); + } + + // Keep this zone - it's a primary (may or may not have had other zones merged into it) + deduplicatedZones.push_back( std::move( aZones[i] ) ); + } + + return deduplicatedZones; +} diff --git a/pcbnew/zone_utils.h b/pcbnew/zone_utils.h new file mode 100644 index 0000000000..4da71f6fa0 --- /dev/null +++ b/pcbnew/zone_utils.h @@ -0,0 +1,39 @@ +/* + * This program source code file is part of KiCad, a free EDA CAD application. + * + * Copyright The KiCad Developers, see AUTHORS.txt for contributors. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 3 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, you may find one here: + * http://www.gnu.org/licenses/gpl-3.0.html + * or you may search the http://www.gnu.org website for the version 3 license, + * or you may write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#pragma once + +#include +#include + +class ZONE; + + +/** + * Merges zones with identical outlines and nets on different layers into single multi-layer zones. + * + * @param aZones is the zones to merge. Ownership of all the zones is taken (some will be returned) + * + * @return the merged zones. Ownership of all the returned zones is transferred to the caller. + */ +std::vector> MergeZonesWithSameOutline( std::vector>&& aZones ); 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 e980c93002..3c632d19a2 100644 --- a/qa/tests/libs/kimath/geometry/test_shape_line_chain.cpp +++ b/qa/tests/libs/kimath/geometry/test_shape_line_chain.cpp @@ -1300,6 +1300,31 @@ BOOST_AUTO_TEST_CASE( CompareGeometry ) } +BOOST_AUTO_TEST_CASE( CompareGeometryReversed ) +{ + // Square + const std::vector ptsA = { + { 0, 0 }, + { 100, 0 }, + { 100, 100 }, + { 0, 100 }, + }; + // Same points, same start, reversed + const std::vector ptsB = { + { 0, 0 }, + { 0, 100 }, + { 100, 100 }, + { 100, 0 }, + }; + + SHAPE_LINE_CHAIN chainA( ptsA, true ); + SHAPE_LINE_CHAIN chainB( ptsB, true ); + + BOOST_TEST( !chainA.CompareGeometry( chainB, false ) ); + BOOST_TEST( chainA.CompareGeometry( chainB, true ) ); +} + + /** * Test for issue #22597: Simplify with tolerance should reduce a polygon * created from a rotated rounded rectangle (many small line segments approximating arcs). diff --git a/qa/tests/pcbnew/test_zone.cpp b/qa/tests/pcbnew/test_zone.cpp index a371fe79cc..82e1bfdc4b 100644 --- a/qa/tests/pcbnew/test_zone.cpp +++ b/qa/tests/pcbnew/test_zone.cpp @@ -26,7 +26,9 @@ #include #include +#include #include +#include struct ZONE_TEST_FIXTURE @@ -35,6 +37,35 @@ struct ZONE_TEST_FIXTURE }; +static std::unique_ptr CreateSquareZone( BOARD_ITEM_CONTAINER& aParent, BOX2I aBox, PCB_LAYER_ID aLayer ) +{ + auto zone = std::make_unique( &aParent ); + zone->SetLayer( aLayer ); + + auto outline = std::make_unique(); + outline->AddOutline( KIGEOM::BoxToLineChain( aBox ) ); + + zone->SetOutline( outline.release() ); + + return zone; +} + + +/** + * Create a similar zone (same outline) on a different layer + */ +static std::unique_ptr CreateSimilarZone( BOARD_ITEM_CONTAINER& aParent, const ZONE& aOther, PCB_LAYER_ID aLayer ) +{ + auto zone = std::make_unique( &aParent ); + zone->SetLayer( aLayer ); + + std::unique_ptr outline = std::make_unique( *aOther.Outline() ); + zone->SetOutline( outline.release() ); + + return zone; +} + + BOOST_FIXTURE_TEST_SUITE( Zone, ZONE_TEST_FIXTURE ) BOOST_AUTO_TEST_CASE( SingleLayer ) @@ -140,4 +171,76 @@ BOOST_AUTO_TEST_CASE( EmptyZoneGetPosition ) BOOST_TEST( zone.GetPosition() == VECTOR2I( 0, 0 ) ); } + +BOOST_AUTO_TEST_CASE( ZoneMergeNull ) +{ + std::vector> zones; + + zones.emplace_back( std::make_unique( &m_board ) ); + zones.back()->SetLayer( F_Cu ); + + zones.emplace_back( std::make_unique( &m_board ) ); + zones.back()->SetLayer( F_Cu ); + + std::vector> merged = MergeZonesWithSameOutline( std::move( zones ) ); + + // They are the same, so they do merge + BOOST_TEST( merged.size() == 1 ); +} + + +BOOST_AUTO_TEST_CASE( ZoneMergeNonNullNoMerge ) +{ + std::vector> zones; + + zones.emplace_back( CreateSquareZone( m_board, BOX2I( VECTOR2I( 0, 0 ), VECTOR2I( 100, 100 ) ), F_Cu ) ); + zones.emplace_back( CreateSquareZone( m_board, BOX2I( VECTOR2I( 200, 200 ), VECTOR2I( 300, 300 ) ), B_Cu ) ); + + std::vector> merged = MergeZonesWithSameOutline( std::move( zones ) ); + + // They are different, so they don't merge + BOOST_TEST( merged.size() == 2 ); +} + + +BOOST_AUTO_TEST_CASE( ZoneMergeNonNullMerge ) +{ + std::vector> zones; + + zones.emplace_back( CreateSquareZone( m_board, BOX2I( VECTOR2I( 0, 0 ), VECTOR2I( 100, 100 ) ), F_Cu ) ); + zones.emplace_back( CreateSimilarZone( m_board, *zones.back(), B_Cu ) ); + + std::vector> merged = MergeZonesWithSameOutline( std::move( zones ) ); + + // They are the same, so they do merge + BOOST_REQUIRE( merged.size() == 1 ); + + BOOST_TEST( merged[0]->GetLayerSet() == ( LSET{ F_Cu, B_Cu } ) ); + BOOST_TEST( merged[0]->GetNumCorners() == 4 ); +} + + +BOOST_AUTO_TEST_CASE( ZoneMergeMergeSameGeomDifferentOrder ) +{ + std::vector> zones; + + zones.emplace_back( CreateSquareZone( m_board, BOX2I( VECTOR2I( 0, 0 ), VECTOR2I( 100, 100 ) ), F_Cu ) ); + zones.emplace_back( CreateSimilarZone( m_board, *zones.back(), B_Cu ) ); + + // Reverse the outline of one of them + // Don't go overboard here - detailed tests of CompareGeometry + // should be in the SHAPE_LINE_CHAIN tests. + auto newPolyB = std::make_unique( *zones.back()->Outline() ); + newPolyB->Outline( 0 ).Reverse(); + zones.back()->SetOutline( newPolyB.release() ); + + std::vector> merged = MergeZonesWithSameOutline( std::move( zones ) ); + + // They are the same, so they do merge + BOOST_REQUIRE( merged.size() == 1 ); + + BOOST_TEST( merged[0]->GetLayerSet() == LSET( { F_Cu, B_Cu } ) ); + BOOST_TEST( merged[0]->GetNumCorners() == 4 ); +} + BOOST_AUTO_TEST_SUITE_END()