Tighten lifecycle management of parent group pointers.

Also adds some debugging to try to catch dangling pointers.

Also adds a cache for group bounding boxes (which will be expensive
to calculate for large groups).

Fixes https://gitlab.com/kicad/code/kicad/issues/12875

include/board_item.h

@@ -66,6 +66,8 @@ public:
     {
     }
 
+    ~BOARD_ITEM();
+
     void SetParentGroup( PCB_GROUP* aGroup ) { m_group = aGroup; }
     PCB_GROUP* GetParentGroup() const { return m_group; }
 
@@ -229,7 +231,7 @@ public:
      *
      * @param aImage the item image which contains data to swap.
      */
-    virtual void SwapData( BOARD_ITEM* aImage );
+    void SwapItemData( BOARD_ITEM* aImage );
 
     /**
      * Test to see if this object is on the given layer.
@@ -321,6 +323,8 @@ protected:
      */
     virtual wxString layerMaskDescribe() const;
 
+    virtual void swapData( BOARD_ITEM* aImage );
+
 protected:
     PCB_LAYER_ID    m_layer;
     bool            m_isKnockout;

include/pcb_group.h

@@ -145,9 +145,6 @@ public:
      */
     PCB_GROUP* DeepDuplicate() const;
 
-    ///< @copydoc BOARD_ITEM::SwapData
-    void SwapData( BOARD_ITEM* aImage ) override;
-
     ///< @copydoc BOARD_ITEM::IsOnLayer
     bool IsOnLayer( PCB_LAYER_ID aLayer ) const override;
 
@@ -221,6 +218,10 @@ public:
      */
     void RunOnDescendants( const std::function<void( BOARD_ITEM* )>& aFunction ) const;
 
+protected:
+    ///< @copydoc BOARD_ITEM::swapData
+    void swapData( BOARD_ITEM* aImage ) override;
+
 private:
     std::unordered_set<BOARD_ITEM*> m_items;     // Members of the group
     wxString                        m_name;      // Optional group name

pcbnew/board.cpp

@@ -120,6 +120,13 @@ BOARD::BOARD() :
 
 BOARD::~BOARD()
 {
+    // Untangle group parents before doing any deleting
+    for( PCB_GROUP* group : m_groups )
+    {
+        for( BOARD_ITEM* item : group->GetItems() )
+            item->SetParentGroup( nullptr );
+    }
+
     // Clean up the owned elements
     DeleteMARKERs();
 
@@ -219,6 +226,7 @@ void BOARD::IncrementTimeStamp()
 
     {
         std::unique_lock<std::mutex> cacheLock( m_CachesMutex );
+
         m_IntersectsAreaCache.clear();
         m_EnclosedByAreaCache.clear();
         m_IntersectsCourtyardCache.clear();
@@ -233,6 +241,7 @@ void BOARD::IncrementTimeStamp()
         m_CopperZoneRTreeCache.clear();
         m_CopperItemRTreeCache = std::make_unique<DRC_RTREE>();
         m_ZoneBBoxCache.clear();
+        m_GroupBBoxCache.clear();
     }
 }
 
@@ -1690,16 +1699,6 @@ void BOARD::GetSortedPadListByXthenYCoord( std::vector<PAD*>& aVector, int aNetC
 }
 
 
-void BOARD::PadDelete( PAD* aPad )
-{
-    GetConnectivity()->Remove( aPad );
-
-    InvokeListeners( &BOARD_LISTENER::OnBoardItemRemoved, *this, aPad );
-
-    aPad->DeleteStructure();
-}
-
-
 std::tuple<int, double, double> BOARD::GetTrackLength( const PCB_TRACK& aTrack ) const
 {
     int    count = 0;

pcbnew/board.h

@@ -1003,14 +1003,6 @@ public:
      */
     PAD* GetPad( std::vector<PAD*>& aPadList, const VECTOR2I& aPosition, LSET aLayerMask ) const;
 
-    /**
-     * Delete a given pad from the BOARD by removing it from its footprint and from the
-     * m_NetInfo.  Makes no UI calls.
-     *
-     * @param aPad is the pad to delete.
-     */
-    void PadDelete( PAD* aPad );
-
     /**
      * First empties then fills the vector with all pads and sorts them by increasing x
      * coordinate, and for increasing y coordinate for same values of x coordinates.  The vector
@@ -1151,6 +1143,7 @@ public:
     std::unordered_map<ZONE*, std::unique_ptr<DRC_RTREE>> m_CopperZoneRTreeCache;
     std::unique_ptr<DRC_RTREE>                            m_CopperItemRTreeCache;
     mutable std::unordered_map<const ZONE*, BOX2I>        m_ZoneBBoxCache;
+    mutable std::unordered_map<const PCB_GROUP*, BOX2I>   m_GroupBBoxCache;
 
     // ------------ DRC caches -------------
     std::vector<ZONE*>    m_DRCZones;

pcbnew/board_commit.cpp

@@ -309,6 +309,9 @@ void BOARD_COMMIT::Push( const wxString& aMessage, int aCommitFlags )
                 itemsDeselected = true;
             }
 
+            if( parentGroup && !( parentGroup->GetFlags() & STRUCT_DELETED ) )
+                parentGroup->RemoveItem( boardItem );
+
             if( autofillZones )
                 dirtyIntersectingZones( boardItem );
 
@@ -339,9 +342,6 @@ void BOARD_COMMIT::Push( const wxString& aMessage, int aCommitFlags )
                         break;
                 }
 
-                if( parentGroup && !( parentGroup->GetFlags() & STRUCT_DELETED ) )
-                    parentGroup->RemoveItem( boardItem );
-
                 if( view )
                     view->Remove( boardItem );
 
@@ -649,7 +649,7 @@ void BOARD_COMMIT::Revert()
             view->Remove( item );
             connectivity->Remove( item );
 
-            item->SwapData( copy );
+            item->SwapItemData( copy );
 
             view->Add( item );
             connectivity->Add( item );

pcbnew/board_item.cpp

@@ -34,6 +34,12 @@
 #include <pcb_group.h>
 
 
+BOARD_ITEM::~BOARD_ITEM()
+{
+    wxASSERT( m_group == nullptr );
+}
+
+
 const BOARD* BOARD_ITEM::GetBoard() const
 {
     if( Type() == PCB_T )
@@ -152,11 +158,32 @@ void BOARD_ITEM::DeleteStructure()
 }
 
 
-void BOARD_ITEM::SwapData( BOARD_ITEM* aImage )
+void BOARD_ITEM::swapData( BOARD_ITEM* aImage )
 {
 }
 
 
+void BOARD_ITEM::SwapItemData( BOARD_ITEM* aImage )
+{
+    if( aImage == nullptr )
+        return;
+
+    wxASSERT( Type() == aImage->Type() );
+    wxASSERT( m_Uuid == aImage->m_Uuid );
+
+    EDA_ITEM*  parent = GetParent();
+    PCB_GROUP* group = GetParentGroup();
+
+    SetParentGroup( nullptr );
+    aImage->SetParentGroup( nullptr );
+    swapData( aImage );
+
+    // Restore pointers to be sure they are not broken
+    SetParent( parent );
+    SetParentGroup( group );
+}
+
+
 BOARD_ITEM* BOARD_ITEM::Duplicate() const
 {
     BOARD_ITEM* dupe = static_cast<BOARD_ITEM*>( Clone() );

pcbnew/edit_zone_helpers.cpp

@@ -45,8 +45,6 @@ void PCB_EDIT_FRAME::Edit_Zone_Params( ZONE* aZone )
 
     // Save initial zones configuration, for undo/redo, before adding new zone
     // note the net name and the layer can be changed, so we must save all zones
-    deletedList.ClearListAndDeleteItems();
-    pickedList.ClearListAndDeleteItems();
     SaveCopyOfZones( pickedList, GetBoard() );
 
     if( aZone->GetIsRuleArea() )
@@ -69,8 +67,8 @@ void PCB_EDIT_FRAME::Edit_Zone_Params( ZONE* aZone )
 
     if( dialogResult == wxID_CANCEL )
     {
-        deletedList.ClearListAndDeleteItems();
-        pickedList.ClearListAndDeleteItems();
+        ClearListAndDeleteItems( &deletedList );
+        ClearListAndDeleteItems( &pickedList );
         return;
     }
 

pcbnew/footprint.cpp

@@ -196,6 +196,13 @@ FOOTPRINT::FOOTPRINT( FOOTPRINT&& aFootprint ) :
 
 FOOTPRINT::~FOOTPRINT()
 {
+    // Untangle group parents before doing any deleting
+    for( PCB_GROUP* group : m_fp_groups )
+    {
+        for( BOARD_ITEM* item : group->GetItems() )
+            item->SetParentGroup( nullptr );
+    }
+
     // Clean up the owned elements
     delete m_reference;
     delete m_value;
@@ -2573,7 +2580,7 @@ void FOOTPRINT::CheckNetTiePadGroups( const std::function<void( const wxString&
 }
 
 
-void FOOTPRINT::SwapData( BOARD_ITEM* aImage )
+void FOOTPRINT::swapData( BOARD_ITEM* aImage )
 {
     wxASSERT( aImage->Type() == PCB_FOOTPRINT_T );
 

pcbnew/footprint.h

@@ -757,7 +757,9 @@ public:
     virtual std::shared_ptr<SHAPE> GetEffectiveShape( PCB_LAYER_ID aLayer = UNDEFINED_LAYER,
             FLASHING aFlash = FLASHING::DEFAULT ) const override;
 
-    virtual void SwapData( BOARD_ITEM* aImage ) override;
+#if defined(DEBUG)
+    virtual void Show( int nestLevel, std::ostream& os ) const override { ShowDummy( os ); }
+#endif
 
     struct cmp_drawings
     {
@@ -774,10 +776,8 @@ public:
         bool operator()( const FP_ZONE* aFirst, const FP_ZONE* aSecond ) const;
     };
 
-
-#if defined(DEBUG)
-    virtual void Show( int nestLevel, std::ostream& os ) const override { ShowDummy( os ); }
-#endif
+protected:
+    virtual void swapData( BOARD_ITEM* aImage ) override;
 
 private:
     DRAWINGS        m_drawings;          // BOARD_ITEMs for drawings on the board, owned by pointer.

pcbnew/pad.cpp

@@ -1541,7 +1541,7 @@ void PAD::ImportSettingsFrom( const PAD& aMasterPad )
 }
 
 
-void PAD::SwapData( BOARD_ITEM* aImage )
+void PAD::swapData( BOARD_ITEM* aImage )
 {
     assert( aImage->Type() == PCB_PAD_T );
 

pcbnew/pad.h

@@ -716,8 +716,6 @@ public:
 
     virtual const BOX2I ViewBBox() const override;
 
-    virtual void SwapData( BOARD_ITEM* aImage ) override;
-
     void ClearZoneConnectionCache()
     {
         for( size_t ii = 0; ii < arrayDim( m_zoneLayerConnections ); ++ii )
@@ -733,6 +731,8 @@ public:
     virtual void Show( int nestLevel, std::ostream& os ) const override { ShowDummy( os ); }
 #endif
 
+protected:
+    virtual void swapData( BOARD_ITEM* aImage ) override;
 
 private:
     void addPadPrimitivesToPolygon( SHAPE_POLY_SET* aMergedPolygon, int aError,

pcbnew/pcb_base_edit_frame.h

@@ -205,6 +205,8 @@ public:
      */
     void ClearUndoORRedoList( UNDO_REDO_LIST whichList, int aItemCount = -1 ) override;
 
+    void ClearListAndDeleteItems( PICKED_ITEMS_LIST* aList );
+
     /**
      * Return the absolute path to the design rules file for the currently-loaded board.
      *

pcbnew/pcb_bitmap.cpp

@@ -94,7 +94,7 @@ EDA_ITEM* PCB_BITMAP::Clone() const
 }
 
 
-void PCB_BITMAP::SwapData( BOARD_ITEM* aItem )
+void PCB_BITMAP::swapData( BOARD_ITEM* aItem )
 {
     wxCHECK_RET( aItem->Type() == PCB_BITMAP_T,
                  wxString::Format( wxT( "PCB_BITMAP object cannot swap data with %s object." ),

pcbnew/pcb_bitmap.h

@@ -85,8 +85,6 @@ public:
     std::shared_ptr<SHAPE> GetEffectiveShape( PCB_LAYER_ID aLayer = UNDEFINED_LAYER,
                                               FLASHING aFlash = FLASHING::DEFAULT ) const override;
 
-    void SwapData( BOARD_ITEM* aItem ) override;
-
     //void Print( const RENDER_SETTINGS* aSettings, const VECTOR2I& aOffset ) override;
 
     /// @copydoc VIEW_ITEM::ViewGetLayers()
@@ -128,6 +126,9 @@ public:
     void Show( int nestLevel, std::ostream& os ) const override;
 #endif
 
+protected:
+    void swapData( BOARD_ITEM* aItem ) override;
+
 private:
     VECTOR2I     m_pos;   // XY coordinates of center of the bitmap
     BITMAP_BASE* m_image; // the BITMAP_BASE item

pcbnew/pcb_dimension.cpp

@@ -560,7 +560,7 @@ EDA_ITEM* PCB_DIM_ALIGNED::Clone() const
 }
 
 
-void PCB_DIM_ALIGNED::SwapData( BOARD_ITEM* aImage )
+void PCB_DIM_ALIGNED::swapData( BOARD_ITEM* aImage )
 {
     wxASSERT( aImage->Type() == Type() );
 
@@ -752,7 +752,7 @@ EDA_ITEM* PCB_DIM_ORTHOGONAL::Clone() const
 }
 
 
-void PCB_DIM_ORTHOGONAL::SwapData( BOARD_ITEM* aImage )
+void PCB_DIM_ORTHOGONAL::swapData( BOARD_ITEM* aImage )
 {
     wxASSERT( aImage->Type() == Type() );
 
@@ -969,7 +969,7 @@ EDA_ITEM* PCB_DIM_LEADER::Clone() const
 }
 
 
-void PCB_DIM_LEADER::SwapData( BOARD_ITEM* aImage )
+void PCB_DIM_LEADER::swapData( BOARD_ITEM* aImage )
 {
     wxASSERT( aImage->Type() == Type() );
 
@@ -1112,7 +1112,7 @@ EDA_ITEM* PCB_DIM_RADIAL::Clone() const
 }
 
 
-void PCB_DIM_RADIAL::SwapData( BOARD_ITEM* aImage )
+void PCB_DIM_RADIAL::swapData( BOARD_ITEM* aImage )
 {
     wxASSERT( aImage->Type() == Type() );
 
@@ -1240,7 +1240,7 @@ EDA_ITEM* PCB_DIM_CENTER::Clone() const
 }
 
 
-void PCB_DIM_CENTER::SwapData( BOARD_ITEM* aImage )
+void PCB_DIM_CENTER::swapData( BOARD_ITEM* aImage )
 {
     wxASSERT( aImage->Type() == Type() );
 

pcbnew/pcb_dimension.h

@@ -358,8 +358,6 @@ public:
 
     EDA_ITEM* Clone() const override;
 
-    virtual void SwapData( BOARD_ITEM* aImage ) override;
-
     void Mirror( const VECTOR2I& axis_pos, bool aMirrorLeftRight = false ) override;
 
     BITMAPS GetMenuImage() const override;
@@ -406,6 +404,8 @@ public:
     void GetMsgPanelInfo( EDA_DRAW_FRAME* aFrame, std::vector<MSG_PANEL_ITEM>& aList ) override;
 
 protected:
+    virtual void swapData( BOARD_ITEM* aImage ) override;
+
     void updateGeometry() override;
 
     void updateText() override;
@@ -445,8 +445,6 @@ public:
 
     EDA_ITEM* Clone() const override;
 
-    void SwapData( BOARD_ITEM* aImage ) override;
-
     BITMAPS GetMenuImage() const override;
 
     /**
@@ -464,6 +462,8 @@ public:
     void     Rotate( const VECTOR2I& aRotCentre, const EDA_ANGLE& aAngle ) override;
 
 protected:
+    void swapData( BOARD_ITEM* aImage ) override;
+
     void updateGeometry() override;
 
     void updateText() override;
@@ -508,8 +508,6 @@ public:
 
     EDA_ITEM* Clone() const override;
 
-    virtual void SwapData( BOARD_ITEM* aImage ) override;
-
     void SetLeaderLength( int aLength ) { m_leaderLength = aLength; }
     int GetLeaderLength() const { return m_leaderLength; }
 
@@ -524,6 +522,8 @@ public:
     }
 
 protected:
+    virtual void swapData( BOARD_ITEM* aImage ) override;
+
     void updateText() override;
     void updateGeometry() override;
 
@@ -559,8 +559,6 @@ public:
 
     EDA_ITEM* Clone() const override;
 
-    virtual void SwapData( BOARD_ITEM* aImage ) override;
-
     BITMAPS GetMenuImage() const override;
 
     wxString GetClass() const override
@@ -574,6 +572,8 @@ public:
     void GetMsgPanelInfo( EDA_DRAW_FRAME* aFrame, std::vector<MSG_PANEL_ITEM>& aList ) override;
 
 protected:
+    virtual void swapData( BOARD_ITEM* aImage ) override;
+
     void updateGeometry() override;
 
 private:
@@ -600,8 +600,6 @@ public:
 
     EDA_ITEM* Clone() const override;
 
-    virtual void SwapData( BOARD_ITEM* aImage ) override;
-
     BITMAPS GetMenuImage() const override;
 
     wxString GetClass() const override
@@ -614,6 +612,8 @@ public:
     const BOX2I ViewBBox() const override;
 
 protected:
+    virtual void swapData( BOARD_ITEM* aImage ) override;
+
     void updateGeometry() override;
 };
 

pcbnew/pcb_group.cpp

@@ -208,7 +208,7 @@ PCB_GROUP* PCB_GROUP::DeepDuplicate() const
 }
 
 
-void PCB_GROUP::SwapData( BOARD_ITEM* aImage )
+void PCB_GROUP::swapData( BOARD_ITEM* aImage )
 {
     assert( aImage->Type() == PCB_GROUP_T );
 
@@ -234,16 +234,41 @@ const BOX2I PCB_GROUP::GetBoundingBox() const
 {
     BOX2I bbox;
 
-    for( BOARD_ITEM* item : m_items )
+    auto calcBBox =
+            [this]()
+            {
+                BOX2I box;
+
+                for( BOARD_ITEM* item : m_items )
+                {
+                    if( item->Type() == PCB_FOOTPRINT_T )
+                        box.Merge( static_cast<FOOTPRINT*>( item )->GetBoundingBox( true, false ) );
+                    else
+                        box.Merge( item->GetBoundingBox() );
+                }
+
+                box.Inflate( pcbIUScale.mmToIU( 0.25 ) ); // Give a min size to the bbox
+
+                return box;
+            };
+
+    if( const BOARD* board = GetBoard() )
     {
-        if( item->Type() == PCB_FOOTPRINT_T )
-            bbox.Merge( static_cast<FOOTPRINT*>( item )->GetBoundingBox( true, false ) );
-        else
-            bbox.Merge( item->GetBoundingBox() );
+        std::unordered_map<const PCB_GROUP*, BOX2I>& cache = board->m_GroupBBoxCache;
+        auto                                         cacheIter = cache.find( this );
+
+        if( cacheIter != cache.end() )
+            return cacheIter->second;
+
+        bbox = calcBBox();
+
+        std::unique_lock<std::mutex> cacheLock( const_cast<BOARD*>( board )->m_CachesMutex );
+        cache[ this ] = bbox;
+
+        return bbox;
     }
 
     bbox.Inflate( pcbIUScale.mmToIU( 0.25 ) ); // Give a min size to the bbox
-
     return bbox;
 }
 

pcbnew/pcb_shape.cpp

@@ -347,7 +347,7 @@ std::shared_ptr<SHAPE> PCB_SHAPE::GetEffectiveShape( PCB_LAYER_ID aLayer, FLASHI
 }
 
 
-void PCB_SHAPE::SwapData( BOARD_ITEM* aImage )
+void PCB_SHAPE::swapData( BOARD_ITEM* aImage )
 {
     PCB_SHAPE* image = dynamic_cast<PCB_SHAPE*>( aImage );
     assert( image );

pcbnew/pcb_shape.h

@@ -145,18 +145,18 @@ public:
     ///< @copydoc VIEW_ITEM::ViewGetLOD
     double ViewGetLOD( int aLayer, KIGFX::VIEW* aView ) const override;
 
-    virtual void SwapData( BOARD_ITEM* aImage ) override;
+#if defined(DEBUG)
+    void Show( int nestLevel, std::ostream& os ) const override { ShowDummy( os ); }
+#endif
+
+protected:
+    virtual void swapData( BOARD_ITEM* aImage ) override;
 
     struct cmp_drawings
     {
         bool operator()( const BOARD_ITEM* aFirst, const BOARD_ITEM* aSecond ) const;
     };
 
-#if defined(DEBUG)
-    void Show( int nestLevel, std::ostream& os ) const override { ShowDummy( os ); }
-#endif
-
-protected:
     EDA_ANGLE getParentOrientation() const override;
     VECTOR2I getParentPosition() const override;
 };

pcbnew/pcb_target.cpp

@@ -138,7 +138,7 @@ EDA_ITEM* PCB_TARGET::Clone() const
 }
 
 
-void PCB_TARGET::SwapData( BOARD_ITEM* aImage )
+void PCB_TARGET::swapData( BOARD_ITEM* aImage )
 {
     assert( aImage->Type() == PCB_TARGET_T );
 

pcbnew/pcb_target.h

@@ -92,8 +92,6 @@ public:
 
     EDA_ITEM* Clone() const override;
 
-    virtual void SwapData( BOARD_ITEM* aImage ) override;
-
     void GetMsgPanelInfo( EDA_DRAW_FRAME* aFrame, std::vector<MSG_PANEL_ITEM>& aList ) override;
 
     /**
@@ -116,6 +114,9 @@ public:
     void Show( int nestLevel, std::ostream& os ) const override { ShowDummy( os ); }
 #endif
 
+protected:
+    virtual void swapData( BOARD_ITEM* aImage ) override;
+
 private:
     int      m_shape;            // bit 0 : 0 = draw +, 1 = draw X
     int      m_size;

pcbnew/pcb_text.cpp

@@ -272,7 +272,7 @@ EDA_ITEM* PCB_TEXT::Clone() const
 }
 
 
-void PCB_TEXT::SwapData( BOARD_ITEM* aImage )
+void PCB_TEXT::swapData( BOARD_ITEM* aImage )
 {
     assert( aImage->Type() == PCB_TEXT_T );
 

pcbnew/pcb_text.h

@@ -147,13 +147,13 @@ public:
 
     EDA_ITEM* Clone() const override;
 
-    virtual void SwapData( BOARD_ITEM* aImage ) override;
-
 #if defined(DEBUG)
     virtual void Show( int nestLevel, std::ostream& os ) const override { ShowDummy( os ); }
 #endif
 
 protected:
+    virtual void swapData( BOARD_ITEM* aImage ) override;
+
     int getKnockoutMargin() const;
 };
 

pcbnew/pcb_textbox.cpp

@@ -430,7 +430,7 @@ EDA_ITEM* PCB_TEXTBOX::Clone() const
 }
 
 
-void PCB_TEXTBOX::SwapData( BOARD_ITEM* aImage )
+void PCB_TEXTBOX::swapData( BOARD_ITEM* aImage )
 {
     assert( aImage->Type() == PCB_TEXTBOX_T );
 

pcbnew/pcb_textbox.h

@@ -135,7 +135,8 @@ public:
 
     EDA_ITEM* Clone() const override;
 
-    virtual void SwapData( BOARD_ITEM* aImage ) override;
+protected:
+    virtual void swapData( BOARD_ITEM* aImage ) override;
 };
 
 #endif  // #define PCB_TEXTBOX_H