Fix layer writing/reading for copper zones

* Always enumerate layers - never use the wildcards
* Keep fills on layers the zone is actually on when loading

Fixes https://gitlab.com/kicad/code/kicad/-/issues/19775

pcbnew/pcb_io/kicad_sexpr/pcb_io_kicad_sexpr.cpp

@@ -1024,7 +1024,7 @@ void PCB_IO_KICAD_SEXPR::format( const PCB_SHAPE* aShape ) const
         KICAD_FORMAT::FormatBool( m_out, "locked", true );
 
     if( aShape->GetLayerSet().count() > 1 )
-        formatLayers( aShape->GetLayerSet() );
+        formatLayers( aShape->GetLayerSet(), false /* enumerate layers */ );
     else
         formatLayer( aShape->GetLayer() );
 
@@ -1363,7 +1363,7 @@ void PCB_IO_KICAD_SEXPR::format( const FOOTPRINT* aFootprint ) const
 }
 
 
-void PCB_IO_KICAD_SEXPR::formatLayers( LSET aLayerMask ) const
+void PCB_IO_KICAD_SEXPR::formatLayers( LSET aLayerMask, bool aEnumerateLayers ) const
 {
     static const LSET cu_all( LSET::AllCuMask( m_board
                                                 ? m_board->GetCopperLayerCount()
@@ -1380,53 +1380,55 @@ void PCB_IO_KICAD_SEXPR::formatLayers( LSET aLayerMask ) const
 
     std::string output;
 
-    // output copper layers first, then non copper
+    if( !aEnumerateLayers )
+    {
+        // output copper layers first, then non copper
+        if( ( aLayerMask & cu_mask ) == cu_mask )
+        {
+            output += ' ' + m_out->Quotew( "*.Cu" );
+            aLayerMask &= ~cu_all;          // clear bits, so they are not output again below
+        }
+        else if( ( aLayerMask & cu_mask ) == fr_bk )
+        {
+            output += ' ' + m_out->Quotew( "F&B.Cu" );
+            aLayerMask &= ~fr_bk;
+        }
 
-    if( ( aLayerMask & cu_mask ) == cu_mask )
-    {
-        output += ' ' + m_out->Quotew( "*.Cu" );
-        aLayerMask &= ~cu_all;          // clear bits, so they are not output again below
-    }
-    else if( ( aLayerMask & cu_mask ) == fr_bk )
-    {
-        output += ' ' + m_out->Quotew( "F&B.Cu" );
-        aLayerMask &= ~fr_bk;
-    }
+        if( ( aLayerMask & adhes ) == adhes )
+        {
+            output += ' ' + m_out->Quotew( "*.Adhes" );
+            aLayerMask &= ~adhes;
+        }
 
-    if( ( aLayerMask & adhes ) == adhes )
-    {
-        output += ' ' + m_out->Quotew( "*.Adhes" );
-        aLayerMask &= ~adhes;
-    }
+        if( ( aLayerMask & paste ) == paste )
+        {
+            output += ' ' + m_out->Quotew( "*.Paste" );
+            aLayerMask &= ~paste;
+        }
 
-    if( ( aLayerMask & paste ) == paste )
-    {
-        output += ' ' + m_out->Quotew( "*.Paste" );
-        aLayerMask &= ~paste;
-    }
+        if( ( aLayerMask & silks ) == silks )
+        {
+            output += ' ' + m_out->Quotew( "*.SilkS" );
+            aLayerMask &= ~silks;
+        }
 
-    if( ( aLayerMask & silks ) == silks )
-    {
-        output += ' ' + m_out->Quotew( "*.SilkS" );
-        aLayerMask &= ~silks;
-    }
+        if( ( aLayerMask & mask ) == mask )
+        {
+            output += ' ' + m_out->Quotew( "*.Mask" );
+            aLayerMask &= ~mask;
+        }
 
-    if( ( aLayerMask & mask ) == mask )
-    {
-        output += ' ' + m_out->Quotew( "*.Mask" );
-        aLayerMask &= ~mask;
-    }
+        if( ( aLayerMask & crt_yd ) == crt_yd )
+        {
+            output += ' ' + m_out->Quotew( "*.CrtYd" );
+            aLayerMask &= ~crt_yd;
+        }
 
-    if( ( aLayerMask & crt_yd ) == crt_yd )
-    {
-        output += ' ' + m_out->Quotew( "*.CrtYd" );
-        aLayerMask &= ~crt_yd;
-    }
-
-    if( ( aLayerMask & fab ) == fab )
-    {
-        output += ' ' + m_out->Quotew( "*.Fab" );
-        aLayerMask &= ~fab;
+        if( ( aLayerMask & fab ) == fab )
+        {
+            output += ' ' + m_out->Quotew( "*.Fab" );
+            aLayerMask &= ~fab;
+        }
     }
 
     // output any individual layers not handled in wildcard combos above
@@ -1551,7 +1553,7 @@ void PCB_IO_KICAD_SEXPR::format( const PAD* aPad ) const
     if( property )
         m_out->Print( "(property %s)", property );
 
-    formatLayers( aPad->GetLayerSet() );
+    formatLayers( aPad->GetLayerSet(), false /* enumerate layers */ );
 
     if( aPad->GetAttribute() == PAD_ATTRIB::PTH )
     {
@@ -2432,7 +2434,7 @@ void PCB_IO_KICAD_SEXPR::format( const PCB_TRACK* aTrack ) const
             KICAD_FORMAT::FormatBool( m_out, "locked", true );
 
         if( aTrack->GetLayerSet().count() > 1 )
-            formatLayers( aTrack->GetLayerSet() );
+            formatLayers( aTrack->GetLayerSet(), false /* enumerate layers */ );
         else
             formatLayer( aTrack->GetLayer() );
 
@@ -2474,8 +2476,9 @@ void PCB_IO_KICAD_SEXPR::format( const ZONE* aZone ) const
     if( aZone->GetBoard() )
         layers &= aZone->GetBoard()->GetEnabledLayers();
 
+    // Always enumerate every layer for a zone on a copper layer
     if( layers.count() > 1 )
-        formatLayers( layers );
+        formatLayers( layers, aZone->IsOnCopperLayer() );
     else
         formatLayer( aZone->GetFirstLayer() );
 

pcbnew/pcb_io/kicad_sexpr/pcb_io_kicad_sexpr.h

@@ -451,7 +451,7 @@ private:
 
     void formatLayer( PCB_LAYER_ID aLayer, bool aIsKnockout = false ) const;
 
-    void formatLayers( LSET aLayerMask ) const;
+    void formatLayers( LSET aLayerMask, bool aEnumerateLayers ) const;
 
     void formatTenting( const PADSTACK& aPadstack ) const;
 

pcbnew/pcb_io/kicad_sexpr/pcb_io_kicad_sexpr_parser.cpp

@@ -1231,7 +1231,7 @@ BOARD* PCB_IO_KICAD_SEXPR_PARSER::parseBOARD_unchecked()
         {
             ZONE* z = static_cast<ZONE*>( zone );
 
-            z->SetLayerSet( z->GetLayerSet() & layers );
+            z->SetLayerSetAndRemoveUnusedFills( z->GetLayerSet() & layers );
         }
     }
 

pcbnew/zone.cpp

@@ -1629,6 +1629,30 @@ void ZONE::TransformSolidAreasShapesToPolygon( PCB_LAYER_ID aLayer, SHAPE_POLY_S
 }
 
 
+void ZONE::SetLayerSetAndRemoveUnusedFills( const LSET& aLayerSet )
+{
+    if( aLayerSet.count() == 0 )
+        return;
+
+    if( m_layerSet != aLayerSet )
+    {
+        aLayerSet.RunOnLayers(
+                [&]( PCB_LAYER_ID layer )
+                {
+                    // Only keep layers that are present in the new set
+                    if( !aLayerSet.Contains( layer ) )
+                    {
+                        m_FilledPolysList[layer]  = std::make_shared<SHAPE_POLY_SET>();
+                        m_filledPolysHash[layer]  = {};
+                        m_insulatedIslands[layer] = {};
+                    }
+                } );
+    }
+
+    m_layerSet = aLayerSet;
+}
+
+
 bool ZONE::operator==( const BOARD_ITEM& aOther ) const
 {
     if( aOther.Type() != Type() )

pcbnew/zone.h

@@ -132,6 +132,13 @@ public:
     void SetLayerSet( const LSET& aLayerSet ) override;
     virtual LSET GetLayerSet() const override { return m_layerSet; }
 
+    /**
+     * Set the zone to be on the aLayerSet layers and only remove the fill polygons
+     * from the unused layers, while keeping the fills on the layers in both the old
+     * and new layer sets.
+     */
+    void SetLayerSetAndRemoveUnusedFills( const LSET& aLayerSet );
+
     const wxString& GetZoneName() const { return m_zoneName; }
     void SetZoneName( const wxString& aName ) { m_zoneName = aName; }
 

qa/tests/pcbnew/CMakeLists.txt

@@ -71,6 +71,8 @@ set( QA_PCBNEW_SRCS
     pcb_io/cadstar/test_cadstar_footprints.cpp
     pcb_io/eagle/test_eagle_lbr_import.cpp
 
+    pcb_io/kicad_sexpr/test_kicad_sexpr.cpp
+
     group_saveload.cpp
 )
 

qa/tests/pcbnew/pcb_io/kicad_sexpr/test_kicad_sexpr.cpp

@@ -0,0 +1,103 @@
+/*
+ * 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 2
+ * 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/old-licenses/gpl-2.0.html
+ * or you may search the http://www.gnu.org website for the version 2 license,
+ * or you may write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
+ */
+
+#include <filesystem>
+#include <string>
+
+#include <pcbnew_utils/board_test_utils.h>
+#include <pcbnew_utils/board_file_utils.h>
+#include <qa_utils/wx_utils/unit_test_utils.h>
+
+#include <pcbnew/pcb_io/kicad_sexpr/pcb_io_kicad_sexpr.h>
+
+#include <board.h>
+#include <zone.h>
+
+
+struct KICAD_SEXPR_FIXTURE
+{
+    KICAD_SEXPR_FIXTURE() {}
+
+    PCB_IO_KICAD_SEXPR kicadPlugin;
+};
+
+
+/**
+ * Declares the struct as the Boost test fixture.
+ */
+BOOST_FIXTURE_TEST_SUITE( KiCadSexprIO, KICAD_SEXPR_FIXTURE )
+
+
+/**
+ * Compare all footprints declared in a *.lbr file with their KiCad reference footprint
+ */
+BOOST_AUTO_TEST_CASE( Issue19775_ZoneLayerWildcards )
+{
+    std::string dataPath = KI_TEST::GetPcbnewTestDataDir() + "plugins/kicad_sexpr/Issue19775_ZoneLayers/";
+
+    BOOST_TEST_CONTEXT( "Zone layers with wildcards" )
+    {
+        std::unique_ptr<BOARD> testBoard = std::make_unique<BOARD>();
+
+        kicadPlugin.LoadBoard( dataPath + "LayerWildcard.kicad_pcb", testBoard.get() );
+
+        // One zone in the file
+        BOOST_CHECK( testBoard->Zones().size() == 1 );
+
+        ZONE* z = testBoard->Zones()[0];
+
+        // On both front and back layers, with zone fill on both
+        BOOST_CHECK( z->GetLayerSet().Contains( F_Cu ) && z->GetLayerSet().Contains( B_Cu ) );
+        BOOST_CHECK( z->GetFilledPolysList( F_Cu )->TotalVertices() > 0 );
+        BOOST_CHECK( z->GetFilledPolysList( B_Cu )->TotalVertices() > 0 );
+    }
+
+    BOOST_TEST_CONTEXT( "Round trip layers" )
+    {
+        auto tmpBoard = std::filesystem::temp_directory_path() / "Issue19775_RoundTrip.kicad_pcb";
+
+        // Load and save the board from above to test how we write the zones into it
+        {
+            std::unique_ptr<BOARD> testBoard = std::make_unique<BOARD>();
+            kicadPlugin.LoadBoard( dataPath + "LayerEnumerate.kicad_pcb", testBoard.get() );
+            kicadPlugin.SaveBoard( tmpBoard.string(), testBoard.get() );
+        }
+
+        // Read the new board
+        std::unique_ptr<BOARD> testBoard = std::make_unique<BOARD>();
+        kicadPlugin.LoadBoard( tmpBoard.string(), testBoard.get() );
+
+        // One zone in the file
+        BOOST_CHECK( testBoard->Zones().size() == 1 );
+
+        ZONE* z = testBoard->Zones()[0];
+
+        // On both front and back layers, with zone fill on both
+        BOOST_CHECK( z->GetLayerSet().Contains( F_Cu ) && z->GetLayerSet().Contains( B_Cu ) );
+        BOOST_CHECK( z->GetFilledPolysList( F_Cu )->TotalVertices() > 0 );
+        BOOST_CHECK( z->GetFilledPolysList( B_Cu )->TotalVertices() > 0 );
+    }
+}
+
+
+BOOST_AUTO_TEST_SUITE_END()