diff --git a/libs/kimath/include/geometry/shape_utils.h b/libs/kimath/include/geometry/shape_utils.h
index a18caffb2c..55fccce5bd 100644
--- a/libs/kimath/include/geometry/shape_utils.h
+++ b/libs/kimath/include/geometry/shape_utils.h
@@ -46,6 +46,7 @@ class HALF_LINE;
class LINE;
class SEG;
class SHAPE_RECT;
+class SHAPE_POLY_SET;
struct TYPED_POINT2I;
namespace KIGEOM
@@ -162,4 +163,19 @@ std::vector<TYPED_POINT2I> GetCircleKeyPoints( const CIRCLE& aCircle, bool aIncl
*/
SHAPE_LINE_CHAIN RectifyPolygon( const SHAPE_LINE_CHAIN& aPoly );
+
+/**
+ * Adds a hole to a polygon if it is valid (i.e. it has 3 or more points
+ * and a non-zero area.)
+ *
+ * This is important if you are reading in a polygon from a file and
+ * aren't sure if it will end up being valid. Leaving invalid holes in
+ * a SHAP_POLY_SET will violate the invariant that holes are non-null.
+ *
+ * @param aOutline is the outline to add the hole to.
+ * @param aHole is the hole to add.
+ * @return true if the hole was added, false if it was not.
+ */
+bool AddHoleIfValid( SHAPE_POLY_SET& aOutline, SHAPE_LINE_CHAIN&& aHole );
+
} // namespace KIGEOM
\ No newline at end of file
diff --git a/libs/kimath/src/geometry/shape_utils.cpp b/libs/kimath/src/geometry/shape_utils.cpp
index dcb6854877..07f46b2e44 100644
--- a/libs/kimath/src/geometry/shape_utils.cpp
+++ b/libs/kimath/src/geometry/shape_utils.cpp
@@ -28,6 +28,7 @@
#include <geometry/half_line.h>
#include <geometry/line.h>
#include <geometry/point_types.h>
+#include <geometry/shape_poly_set.h>
#include <geometry/shape_rect.h>
@@ -375,4 +376,16 @@ SHAPE_LINE_CHAIN KIGEOM::RectifyPolygon( const SHAPE_LINE_CHAIN& aPoly )
raOutline.Simplify();
return raOutline;
-}
\ No newline at end of file
+}
+
+
+bool KIGEOM::AddHoleIfValid( SHAPE_POLY_SET& aOutline, SHAPE_LINE_CHAIN&& aHole )
+{
+ if( aHole.PointCount() < 3 || aHole.Area() == 0 )
+ {
+ return false;
+ }
+
+ aOutline.AddHole( std::move( aHole ) );
+ return true;
+}
diff --git a/pcbnew/pcb_io/fabmaster/import_fabmaster.cpp b/pcbnew/pcb_io/fabmaster/import_fabmaster.cpp
index 8bdc2b1e21..1df674f931 100644
--- a/pcbnew/pcb_io/fabmaster/import_fabmaster.cpp
+++ b/pcbnew/pcb_io/fabmaster/import_fabmaster.cpp
@@ -50,9 +50,11 @@
#include <zone.h>
#include <common.h>
#include <geometry/shape_arc.h>
+#include <geometry/shape_utils.h>
#include <string_utils.h>
#include <progress_reporter.h>
#include <math/util.h>
+
#include <wx/filename.h>
@@ -2790,8 +2792,6 @@ bool FABMASTER::loadZone( BOARD* aBoard, const std::unique_ptr<FABMASTER::TRACE>
if( aLine->segment.size() < 3 )
return false;
- int last_subseq = 0;
- int hole_idx = -1;
SHAPE_POLY_SET* zone_outline = nullptr;
ZONE* zone = nullptr;
@@ -2841,7 +2841,29 @@ bool FABMASTER::loadZone( BOARD* aBoard, const std::unique_ptr<FABMASTER::TRACE>
zone_outline->NewOutline();
+ std::unique_ptr<SHAPE_LINE_CHAIN> pending_hole = nullptr;
+ SHAPE_LINE_CHAIN* active_chain = &zone_outline->Outline( 0 );
+ const auto add_hole_if_valid = [&]()
+ {
+ if( pending_hole )
+ {
+ pending_hole->SetClosed( true );
+
+ // If we get junk holes, assert, but don't add them to the zone, as that
+ // will cause crashes later.
+ if( !KIGEOM::AddHoleIfValid( *zone_outline, std::move( *pending_hole ) ) )
+ {
+ wxLogMessage( _( "Invalid hole with %d points in zone on layer %s with net %s" ),
+ pending_hole->PointCount(), zone->GetLayerName(),
+ zone->GetNetname() );
+ }
+
+ pending_hole.reset();
+ }
+ };
+
+ int last_subseq = 0;
for( const auto& seg : aLine->segment )
{
if( seg->subseq > 0 && seg->subseq != last_subseq )
@@ -2851,33 +2873,52 @@ bool FABMASTER::loadZone( BOARD* aBoard, const std::unique_ptr<FABMASTER::TRACE>
if( aLine->lclass == "BOUNDARY" )
break;
- hole_idx = zone_outline->AddHole( SHAPE_LINE_CHAIN{} );
- last_subseq = seg->subseq;
+ add_hole_if_valid();
+ pending_hole = std::make_unique<SHAPE_LINE_CHAIN>();
+ active_chain = pending_hole.get();
last_subseq = seg->subseq;
}
if( seg->shape == GR_SHAPE_LINE )
{
const GRAPHIC_LINE* src = static_cast<const GRAPHIC_LINE*>( seg.get() );
+ const VECTOR2I start( src->start_x, src->start_y );
+ const VECTOR2I end( src->end_x, src->end_y );
- if( zone_outline->VertexCount( 0, hole_idx ) == 0 )
- zone_outline->Append( src->start_x, src->start_y, 0, hole_idx );
+ if( active_chain->PointCount() == 0 )
+ {
+ active_chain->Append( start );
+ }
+ else
+ {
+ const VECTOR2I& last = active_chain->CPoint( -1 );
- zone_outline->Append( src->end_x, src->end_y, 0, hole_idx );
+ // Not if this can ever happen, or what do if it does (add both points?).
+ if( last != start )
+ {
+ wxLogError( _( "Outline seems discontinuous: last point was %s, "
+ "start point of next segment is %s" ),
+ last.Format(), start.Format() );
+ }
+ }
+
+ active_chain->Append( end );
}
else if( seg->shape == GR_SHAPE_ARC || seg->shape == GR_SHAPE_CIRCLE )
{
/* Even if it says "circle", it's actually an arc, it's just closed */
const GRAPHIC_ARC* src = static_cast<const GRAPHIC_ARC*>( seg.get() );
- zone_outline->Hole( 0, hole_idx ).Append( src->result );
+ active_chain->Append( src->result );
}
else
{
- wxASSERT_MSG( false,
- wxString::Format( "Invalid shape type %d in zone outline", seg->shape ) );
+ wxLogError( _( "Invalid shape type %d in zone outline" ), seg->shape );
}
}
+ // Finalise the last hole, if any
+ add_hole_if_valid();
+
if( zone_outline->Outline( 0 ).PointCount() >= 3 )
{
zone->SetOutline( zone_outline );