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 <[email protected]> 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
_______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
