kicad-developers team mailing list archive

[Oliver Walters <oliver.henry.walters@xxxxxxxxx>]

Hey all,

I noticed that the Duplicate functionality in pcbnew / modedit was woefully
slow when copying more than a handful of items.

I benchmarked it with 1000 items, it took 58 seconds to perform the
duplication (KiCAD was at 100% CPU the whole time).

I have attached a patch that reduces this time to ~200ms for the same set
of items.

The speed issue is due to each item being passed through the tool framework
multiple times. The patch adds a tool function to select / deselect a list
of items with a single call

Regards,
Oliver

From 70822a2d7e831a047d98552ff4faf7eef8bc884a Mon Sep 17 00:00:00 2001
From: Oliver Walters <oliver.henry.walters@xxxxxxxxx>
Date: Sun, 28 May 2017 22:33:43 +1000
Subject: [PATCH] Improved speed of Duplicate action

- Removed repetitive tool calls
---
 pcbnew/tools/edit_tool.cpp      | 56 +++++++++++++++++++-------------
 pcbnew/tools/pcb_actions.h      |  6 ++++
 pcbnew/tools/selection_tool.cpp | 72 ++++++++++++++++++++++++++++++++++++++++-
 pcbnew/tools/selection_tool.h   |  6 ++++
 4 files changed, 116 insertions(+), 24 deletions(-)

diff --git a/pcbnew/tools/edit_tool.cpp b/pcbnew/tools/edit_tool.cpp
index 2211fed..73a5907 100644
--- a/pcbnew/tools/edit_tool.cpp
+++ b/pcbnew/tools/edit_tool.cpp
@@ -297,7 +297,9 @@ int EDIT_TOOL::Main( const TOOL_EVENT& aEvent )
 
                 // Drag items to the current cursor position
                 for( auto item : selection )
+                {
                     static_cast<BOARD_ITEM*>( item )->Move( movement + m_offset );
+                }
             }
             else if( !m_dragging )    // Prepare to start dragging
             {
@@ -828,10 +830,14 @@ int EDIT_TOOL::MoveExact( const TOOL_EVENT& aEvent )
 }
 
 
+/**
+ * Duplicate items that are currently selected (by SELECTION_TOOL)
+ *
+ * Each selected item is duplicated and pushed to new_items list
+ * Old selection is cleared, and new items are then selected.
+ */
 int EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent )
 {
-    // Note: original items are no more modified.
-
     bool increment = aEvent.IsAction( &PCB_ACTIONS::duplicateIncrement );
 
     // Be sure that there is at least one item that we can modify
@@ -843,28 +849,24 @@ int EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent )
     // we have a selection to work on now, so start the tool process
     PCB_BASE_EDIT_FRAME* editFrame = getEditFrame<PCB_BASE_EDIT_FRAME>();
 
-    std::vector<BOARD_ITEM*> old_items;
+    std::vector<BOARD_ITEM*> new_items;
 
-    for( auto item : selection )
-    {
-        if( item )
-            old_items.push_back( static_cast<BOARD_ITEM*>( item ) );
-    }
+    BOARD_ITEM* orig_item = nullptr;
+    BOARD_ITEM* dupe_item = nullptr;
 
-    for( unsigned i = 0; i < old_items.size(); ++i )
+    // Duplicate each selected item
+    for( auto item : selection )
     {
-        BOARD_ITEM* item = old_items[i];
-
-        // Unselect the item, so we won't pick it up again
-        // Do this first, so a single-item duplicate will correctly call
-        // SetCurItem and show the item properties
-        m_toolMgr->RunAction( PCB_ACTIONS::unselectItem, true, item );
+        orig_item = static_cast<BOARD_ITEM*>( item );
 
-        BOARD_ITEM* new_item = NULL;
+        if( !orig_item )
+        {
+            continue;
+        }
 
         if( m_editModules )
         {
-            new_item = editFrame->GetBoard()->m_Modules->Duplicate( item, increment );
+            dupe_item = editFrame->GetBoard()->m_Modules->Duplicate( orig_item, increment );
         }
         else
         {
@@ -874,23 +876,31 @@ int EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent )
             // so zones are not duplicated
             if( item->Type() != PCB_ZONE_AREA_T )
 #endif
-            new_item = editFrame->GetBoard()->Duplicate( item );
+            dupe_item = editFrame->GetBoard()->Duplicate( orig_item );
         }
 
-        if( new_item )
+        if( dupe_item )
         {
-            m_commit->Add( new_item );
+            // Clear the selection flag here, otherwise the SELECTION_TOOL
+            // will not properly select it later on
+            dupe_item->ClearSelected();
 
-            // Select the new item, so we can pick it up
-            m_toolMgr->RunAction( PCB_ACTIONS::selectItem, true, new_item );
+            new_items.push_back( dupe_item );
+            m_commit->Add( dupe_item );
         }
     }
 
+    // Clear the old selection first
+    m_toolMgr->RunAction( PCB_ACTIONS::selectionClear, true );
+
+    // Select the new items
+    m_toolMgr->RunAction( PCB_ACTIONS::selectItems, true, &new_items );
+
     // record the new items as added
     if( !selection.Empty() )
     {
         editFrame->DisplayToolMsg( wxString::Format( _( "Duplicated %d item(s)" ),
-                (int) old_items.size() ) );
+                (int) new_items.size() ) );
 
         // If items were duplicated, pick them up
         // this works well for "dropping" copies around and pushes the commit
diff --git a/pcbnew/tools/pcb_actions.h b/pcbnew/tools/pcb_actions.h
index 59bf543..03d3b3f 100644
--- a/pcbnew/tools/pcb_actions.h
+++ b/pcbnew/tools/pcb_actions.h
@@ -55,9 +55,15 @@ public:
     /// Selects an item (specified as the event parameter).
     static TOOL_ACTION selectItem;
 
+    /// Selects a list of items (specified as the event parameter)
+    static TOOL_ACTION selectItems;
+
     /// Unselects an item (specified as the event parameter).
     static TOOL_ACTION unselectItem;
 
+    /// Unselects a list of items (specified as the event parameter)
+    static TOOL_ACTION unselectItems;
+
     /// Selects a connection between junctions.
     static TOOL_ACTION selectConnection;
 
diff --git a/pcbnew/tools/selection_tool.cpp b/pcbnew/tools/selection_tool.cpp
index dd339f3..23d83d9 100644
--- a/pcbnew/tools/selection_tool.cpp
+++ b/pcbnew/tools/selection_tool.cpp
@@ -70,10 +70,18 @@ TOOL_ACTION PCB_ACTIONS::selectItem( "pcbnew.InteractiveSelection.SelectItem",
         AS_GLOBAL, 0,
         "", "" );    // No description, it is not supposed to be shown anywhere
 
+TOOL_ACTION PCB_ACTIONS::selectItems( "pcbnew.InteractiveSelection.SelectItems",
+        AS_GLOBAL, 0,
+        "", "" );    // No description, it is not supposed to be shown anywhere
+
 TOOL_ACTION PCB_ACTIONS::unselectItem( "pcbnew.InteractiveSelection.UnselectItem",
         AS_GLOBAL, 0,
         "", "" );    // No description, it is not supposed to be shown anywhere
 
+TOOL_ACTION PCB_ACTIONS::unselectItems( "pcbnew.InteractiveSelection.UnselectItems",
+        AS_GLOBAL, 0,
+        "", "" );    // No description, it is not supposed to be shown anywhere
+
 TOOL_ACTION PCB_ACTIONS::selectionClear( "pcbnew.InteractiveSelection.Clear",
         AS_GLOBAL, 0,
         "", "" );    // No description, it is not supposed to be shown anywhere
@@ -235,7 +243,7 @@ int SELECTION_TOOL::Main( const TOOL_EVENT& aEvent )
         // This will be ignored if the SHIFT modifier is pressed
         m_subtractive = !m_additive && evt->Modifier( MD_CTRL );
 
-        // single click? Select single object
+        // Single click? Select single object
         if( evt->IsClick( BUT_LEFT ) )
         {
             if( evt->Modifier( MD_CTRL ) && !m_editModules )
@@ -244,8 +252,11 @@ int SELECTION_TOOL::Main( const TOOL_EVENT& aEvent )
             }
             else
             {
+                // If no modifier keys are pressed, clear the selection
                 if( !m_additive )
+                {
                     clearSelection();
+                }
 
                 selectPoint( evt->Position() );
             }
@@ -585,7 +596,9 @@ void SELECTION_TOOL::SetTransitions()
     Go( &SELECTION_TOOL::CursorSelection, PCB_ACTIONS::selectionCursor.MakeEvent() );
     Go( &SELECTION_TOOL::ClearSelection, PCB_ACTIONS::selectionClear.MakeEvent() );
     Go( &SELECTION_TOOL::SelectItem, PCB_ACTIONS::selectItem.MakeEvent() );
+    Go( &SELECTION_TOOL::SelectItems, PCB_ACTIONS::selectItems.MakeEvent() );
     Go( &SELECTION_TOOL::UnselectItem, PCB_ACTIONS::unselectItem.MakeEvent() );
+    Go( &SELECTION_TOOL::UnselectItems, PCB_ACTIONS::unselectItems.MakeEvent() );
     Go( &SELECTION_TOOL::find, PCB_ACTIONS::find.MakeEvent() );
     Go( &SELECTION_TOOL::findMove, PCB_ACTIONS::findMove.MakeEvent() );
     Go( &SELECTION_TOOL::filterSelection, PCB_ACTIONS::filterSelection.MakeEvent() );
@@ -672,6 +685,33 @@ int SELECTION_TOOL::ClearSelection( const TOOL_EVENT& aEvent )
     return 0;
 }
 
+
+/**
+ * Select multiple items (vector passed as event parameter)
+ */
+int SELECTION_TOOL::SelectItems( const TOOL_EVENT& aEvent )
+{
+    std::vector<BOARD_ITEM*>* items = aEvent.Parameter<std::vector<BOARD_ITEM*>*>();
+
+    if( items )
+    {
+        // Perform individual selection of each item
+        // before processing the event.
+        for( auto item : *items )
+        {
+            select( item );
+        }
+
+        m_toolMgr->ProcessEvent( SelectedEvent );
+    }
+
+    return 0;
+}
+
+
+/**
+ * Select a single item (passed as event parameter)
+ */
 int SELECTION_TOOL::SelectItem( const TOOL_EVENT& aEvent )
 {
     // Check if there is an item to be selected
@@ -689,6 +729,31 @@ int SELECTION_TOOL::SelectItem( const TOOL_EVENT& aEvent )
 }
 
 
+/**
+ * Unselect multiple items (vector passed as event parameter)
+ */
+int SELECTION_TOOL::UnselectItems( const TOOL_EVENT& aEvent )
+{
+    std::vector<BOARD_ITEM*>* items = aEvent.Parameter<std::vector<BOARD_ITEM*>*>();
+
+    if( items )
+    {
+        // Perform individual unselection of each item
+        // before processing the event
+        for( auto item : *items )
+        {
+            unselect( item );
+        }
+
+        m_toolMgr->ProcessEvent( UnselectedEvent );
+    }
+
+    return 0;
+}
+
+/**
+ * Unselect a single item (passed as event parameter)
+ */
 int SELECTION_TOOL::UnselectItem( const TOOL_EVENT& aEvent )
 {
     // Check if there is an item to be selected
@@ -1165,10 +1230,14 @@ int SELECTION_TOOL::filterSelection( const TOOL_EVENT& aEvent )
 void SELECTION_TOOL::clearSelection()
 {
     if( m_selection.Empty() )
+    {
         return;
+    }
 
     for( auto item : m_selection )
+    {
         unselectVisually( static_cast<BOARD_ITEM*>( item ) );
+    }
 
     m_selection.Clear();
 
@@ -1393,6 +1462,7 @@ bool SELECTION_TOOL::selectable( const BOARD_ITEM* aItem ) const
     return board()->IsLayerVisible( aItem->GetLayer() );
 }
 
+
 void SELECTION_TOOL::select( BOARD_ITEM* aItem )
 {
     if( aItem->IsSelected() )
diff --git a/pcbnew/tools/selection_tool.h b/pcbnew/tools/selection_tool.h
index 642842d..dab2f5b 100644
--- a/pcbnew/tools/selection_tool.h
+++ b/pcbnew/tools/selection_tool.h
@@ -114,9 +114,15 @@ public:
     ///> Item selection event handler.
     int SelectItem( const TOOL_EVENT& aEvent );
 
+    ///> Multiple item selection event handler
+    int SelectItems( const TOOL_EVENT& aEvent );
+
     ///> Item unselection event handler.
     int UnselectItem( const TOOL_EVENT& aEvent );
 
+    ///> Multiple item unselection event handler
+    int UnselectItems( const TOOL_EVENT& aEvent );
+
     ///> Event sent after an item is selected.
     static const TOOL_EVENT SelectedEvent;
 
-- 
2.7.4