kicad-developers team mailing list archive

[Robbert Lagerweij <rlagerweij@xxxxxxxxxxx>]

Hi all,


While writing the documentation for the new origin selection feature in the move exactly tool, I found some ways to improve the consistency of its behaviour. I then could not help but also improve the structure of the code a bit while I was at it.


These two patches together do the following:

* move the determination of the origin coordinates to the dialog (reduces code duplication across tools somewhat)
* extract a method to determine the anchor point of the move to reduce the size of the move exact method
* improve the logic of the anchor point determination for selections of multiple objects which do not contain footprints

Happy to receive any and all comments and suggestions for improvements.

Kind regards,

Robbert


From 20bd08c556d4a03802271fc5979d8d164328d68d Mon Sep 17 00:00:00 2001
From: Robbert Lagerweij <rlagerweij@xxxxxxxxxxx>
Date: Thu, 22 Jun 2017 21:25:40 +0200
Subject: [PATCH 1/2] pcbnew - refactor move exactly tool (1/2)

This patch does the following:
* move the determination of the origin coordinates to the dialog (reduces code duplication)
---
 pcbnew/block_module_editor.cpp       | 27 ++-------------------------
 pcbnew/dialogs/dialog_move_exact.cpp | 31 ++++++++++++++++++++++++++++++-
 pcbnew/dialogs/dialog_move_exact.h   |  1 +
 pcbnew/edit.cpp                      | 27 ++-------------------------
 pcbnew/modedit.cpp                   | 26 ++------------------------
 pcbnew/tools/edit_tool.cpp           | 30 +-----------------------------
 6 files changed, 38 insertions(+), 104 deletions(-)

diff --git a/pcbnew/block_module_editor.cpp b/pcbnew/block_module_editor.cpp
index 32405e8..d1dcf5c 100644
--- a/pcbnew/block_module_editor.cpp
+++ b/pcbnew/block_module_editor.cpp
@@ -200,35 +200,12 @@ bool FOOTPRINT_EDIT_FRAME::HandleBlockEnd( wxDC* DC )
                 SaveCopyInUndoList( currentModule, UR_CHANGED );
                 wxPoint blockCentre = GetScreen()->m_BlockLocate.Centre();
 
-                wxPoint origin;
-
-                switch( params.origin )
+                if( params.origin == RELATIVE_TO_CURRENT_POSITION )
                 {
-                case RELATIVE_TO_USER_ORIGIN:
-                    origin = GetScreen()->m_O_Curseur;
-                    break;
-
-                case RELATIVE_TO_GRID_ORIGIN:
-                    origin = GetGridOrigin();
-                    break;
-
-                case RELATIVE_TO_DRILL_PLACE_ORIGIN:
-                    origin = GetAuxOrigin();
-                    break;
-
-                case RELATIVE_TO_SHEET_ORIGIN:
-                    origin = wxPoint( 0, 0 );
-                    break;
-
-                case RELATIVE_TO_CURRENT_POSITION:
-                    // relative movement means that only the translation values should be used:
-                    // -> set origin and blockCentre to zero
-                    origin = wxPoint( 0, 0 );
                     blockCentre = wxPoint( 0, 0 );
-                    break;
                 }
 
-                wxPoint finalMoveVector = params.translation + origin - blockCentre;
+                wxPoint finalMoveVector = params.translation - blockCentre;
 
                 MoveMarkedItemsExactly( currentModule, blockCentre, finalMoveVector, params.rotation );
             }
diff --git a/pcbnew/dialogs/dialog_move_exact.cpp b/pcbnew/dialogs/dialog_move_exact.cpp
index 1736086..f2f2ae0 100644
--- a/pcbnew/dialogs/dialog_move_exact.cpp
+++ b/pcbnew/dialogs/dialog_move_exact.cpp
@@ -37,6 +37,7 @@ DIALOG_MOVE_EXACT::MOVE_EXACT_OPTIONS DIALOG_MOVE_EXACT::m_options;
 
 DIALOG_MOVE_EXACT::DIALOG_MOVE_EXACT(PCB_BASE_FRAME *aParent, MOVE_PARAMETERS &aParams ) :
     DIALOG_MOVE_EXACT_BASE( aParent ),
+    m_parent( aParent ),
     m_translation( aParams.translation ),
     m_rotation( aParams.rotation ),
     m_origin( aParams.origin ),
@@ -283,8 +284,36 @@ void DIALOG_MOVE_EXACT::OnOkClick( wxCommandEvent& event )
         m_anchor = ANCHOR_FROM_LIBRARY;
     }
 
+    wxPoint move_vector, origin;
     // for the output, we only deliver a Cartesian vector
-    bool ok = GetTranslationInIU( m_translation, m_polarCoords->IsChecked() );
+    bool ok = GetTranslationInIU( move_vector, m_polarCoords->IsChecked() );
+
+    switch( m_origin )
+    {
+    case RELATIVE_TO_USER_ORIGIN:
+        origin = m_parent->GetScreen()->m_O_Curseur;
+        break;
+
+    case RELATIVE_TO_GRID_ORIGIN:
+        origin = m_parent->GetGridOrigin();
+        break;
+
+    case RELATIVE_TO_DRILL_PLACE_ORIGIN:
+        origin = m_parent->GetAuxOrigin();
+        break;
+
+    case RELATIVE_TO_SHEET_ORIGIN:
+        origin = wxPoint( 0, 0 );
+        break;
+
+    case RELATIVE_TO_CURRENT_POSITION:
+        // relative movement means that only the translation values should be used:
+        // -> set origin and anchor to zero
+        origin = wxPoint( 0, 0 );
+        break;
+        }
+
+    m_translation = move_vector + origin;
 
     if( ok )
     {
diff --git a/pcbnew/dialogs/dialog_move_exact.h b/pcbnew/dialogs/dialog_move_exact.h
index f851b2d..6804aab 100644
--- a/pcbnew/dialogs/dialog_move_exact.h
+++ b/pcbnew/dialogs/dialog_move_exact.h
@@ -63,6 +63,7 @@ class DIALOG_MOVE_EXACT : public DIALOG_MOVE_EXACT_BASE
 {
 private:
 
+    PCB_BASE_FRAME*     m_parent;
     wxPoint&            m_translation;
     double&             m_rotation;
     MOVE_EXACT_ORIGIN&  m_origin;
diff --git a/pcbnew/edit.cpp b/pcbnew/edit.cpp
index 5d7cd13..4708f1d 100644
--- a/pcbnew/edit.cpp
+++ b/pcbnew/edit.cpp
@@ -1584,35 +1584,12 @@ void PCB_EDIT_FRAME::moveExact()
                 }
             }
 
-            wxPoint origin;
-
-            switch( params.origin )
+            if( params.origin == RELATIVE_TO_CURRENT_POSITION )
             {
-            case RELATIVE_TO_USER_ORIGIN:
-                origin = GetScreen()->m_O_Curseur;
-                break;
-
-            case RELATIVE_TO_GRID_ORIGIN:
-                origin = GetGridOrigin();
-                break;
-
-            case RELATIVE_TO_DRILL_PLACE_ORIGIN:
-                origin = GetAuxOrigin();
-                break;
-
-            case RELATIVE_TO_SHEET_ORIGIN:
-                origin = wxPoint( 0, 0 );
-                break;
-
-            case RELATIVE_TO_CURRENT_POSITION:
-                // relative movement means that only the translation values should be used:
-                // -> set origin and anchor to zero
-                origin = wxPoint( 0, 0 );
                 anchorPoint = wxPoint( 0, 0 );
-                break;
             }
 
-            wxPoint finalMoveVector = params.translation + origin - anchorPoint;
+            wxPoint finalMoveVector = params.translation - anchorPoint;
 
             item->Move( finalMoveVector );
             item->Rotate( item->GetPosition(), params.rotation );
diff --git a/pcbnew/modedit.cpp b/pcbnew/modedit.cpp
index 37917d8..69ae127 100644
--- a/pcbnew/modedit.cpp
+++ b/pcbnew/modedit.cpp
@@ -852,35 +852,13 @@ void FOOTPRINT_EDIT_FRAME::moveExact()
         BOARD_ITEM* item = GetScreen()->GetCurItem();
 
         wxPoint anchorPoint = item->GetPosition();
-        wxPoint origin;
 
-        switch( params.origin )
+        if( params.origin == RELATIVE_TO_CURRENT_POSITION )
         {
-        case RELATIVE_TO_USER_ORIGIN:
-            origin = GetScreen()->m_O_Curseur;
-            break;
-
-        case RELATIVE_TO_GRID_ORIGIN:
-            origin = GetGridOrigin();
-            break;
-
-        case RELATIVE_TO_DRILL_PLACE_ORIGIN:
-            origin = GetAuxOrigin();
-            break;
-
-        case RELATIVE_TO_SHEET_ORIGIN:
-            origin = wxPoint( 0, 0 );
-            break;
-
-        case RELATIVE_TO_CURRENT_POSITION:
-            // relative movement means that only the translation values should be used:
-            // -> set origin and anchor to zero
-            origin = wxPoint( 0, 0 );
             anchorPoint = wxPoint( 0, 0 );
-            break;
         }
 
-        wxPoint finalMoveVector = params.translation + origin - anchorPoint;
+        wxPoint finalMoveVector = params.translation - anchorPoint;
 
         item->Move( finalMoveVector );
         item->Rotate( item->GetPosition(), params.rotation );
diff --git a/pcbnew/tools/edit_tool.cpp b/pcbnew/tools/edit_tool.cpp
index 99d5afd..3e04adf 100644
--- a/pcbnew/tools/edit_tool.cpp
+++ b/pcbnew/tools/edit_tool.cpp
@@ -776,35 +776,7 @@ int EDIT_TOOL::MoveExact( const TOOL_EVENT& aEvent )
             }
         }
 
-        wxPoint origin;
-
-        switch( params.origin )
-        {
-        case RELATIVE_TO_USER_ORIGIN:
-            origin = editFrame->GetScreen()->m_O_Curseur;
-            break;
-
-        case RELATIVE_TO_GRID_ORIGIN:
-            origin = editFrame->GetGridOrigin();
-            break;
-
-        case RELATIVE_TO_DRILL_PLACE_ORIGIN:
-            origin = editFrame->GetAuxOrigin();
-            break;
-
-        case RELATIVE_TO_SHEET_ORIGIN:
-            origin = wxPoint( 0, 0 );
-            break;
-
-        case RELATIVE_TO_CURRENT_POSITION:
-            // relative movement means that only the translation values should be used:
-            // -> set origin and anchor to zero
-            origin = wxPoint( 0, 0 );
-            anchorPoint = wxPoint( 0, 0 );
-            break;
-        }
-
-        wxPoint finalMoveVector = params.translation + origin - anchorPoint;
+        wxPoint finalMoveVector = params.translation - anchorPoint;
 
         // Make sure the rotation is from the right reference point
         rotPoint += finalMoveVector;
-- 
2.7.4


From c6c7ee18864ace74f44737461c34fac81f033a36 Mon Sep 17 00:00:00 2001
From: Robbert Lagerweij <rlagerweij@xxxxxxxxxxx>
Date: Thu, 22 Jun 2017 21:30:23 +0200
Subject: [PATCH 2/2] pcbnew - refactor move exactly tool (2/2)

This patch does the following:
* extracts a method to determine the anchor point of the move
* improves the logic of the anchor point determination for selections of multiple objects which do not contain footprints
---
 pcbnew/router/CMakeLists.txt |   1 +
 pcbnew/tools/edit_tool.cpp   | 121 +++++++++++++++++++++++++------------------
 pcbnew/tools/edit_tool.h     |   3 ++
 3 files changed, 76 insertions(+), 49 deletions(-)

diff --git a/pcbnew/router/CMakeLists.txt b/pcbnew/router/CMakeLists.txt
index be9b7c3..a5c1152 100644
--- a/pcbnew/router/CMakeLists.txt
+++ b/pcbnew/router/CMakeLists.txt
@@ -7,6 +7,7 @@ include_directories(
     ../../3d-viewer
     ../../pcbnew
     ../../polygon
+	../dialogs
     ${INC_AFTER}
 )
 
diff --git a/pcbnew/tools/edit_tool.cpp b/pcbnew/tools/edit_tool.cpp
index 3e04adf..5775d03 100644
--- a/pcbnew/tools/edit_tool.cpp
+++ b/pcbnew/tools/edit_tool.cpp
@@ -726,55 +726,7 @@ int EDIT_TOOL::MoveExact( const TOOL_EVENT& aEvent )
         VECTOR2I rp = selection.GetCenter();
         wxPoint rotPoint( rp.x, rp.y );
 
-        // Begin at the center of the selection determined above
-        wxPoint anchorPoint = rotPoint;
-
-        // If the anchor is not ANCHOR_FROM_LIBRARY then the user applied an override.
-        // Also run through this block if only one item is slected because it may be a module,
-        // in which case we want something different than the center of the selection
-        if( ( params.anchor != ANCHOR_FROM_LIBRARY ) || ( selection.GetSize() == 1 ) )
-        {
-            BOARD_ITEM* topLeftItem = static_cast<BOARD_ITEM*>( selection.GetTopLeftModule() );
-
-            // no module found if the GetTopLeftModule() returns null, retry for
-            if( topLeftItem == nullptr )
-            {
-                topLeftItem = static_cast<BOARD_ITEM*>( selection.GetTopLeftItem() );
-                anchorPoint = topLeftItem->GetPosition();
-            }
-
-            if( topLeftItem->Type() == PCB_MODULE_T )
-            {
-                // Cast to module to allow access to the pads
-                MODULE* mod = static_cast<MODULE*>( topLeftItem );
-
-                switch( params.anchor )
-                {
-                case ANCHOR_FROM_LIBRARY:
-                    anchorPoint = mod->GetPosition();
-                    break;
-                case ANCHOR_TOP_LEFT_PAD:
-                    topLeftItem = mod->GetTopLeftPad();
-                    break;
-                case ANCHOR_CENTER_FOOTPRINT:
-                    anchorPoint = mod->GetFootprintRect().GetCenter();
-                    break;
-                }
-            }
-
-            if( topLeftItem->Type() == PCB_PAD_T )
-            {
-                if( static_cast<D_PAD*>( topLeftItem )->GetAttribute() == PAD_ATTRIB_SMD )
-                {
-                    // Use the top left corner of SMD pads as an anchor instead of the center
-                    anchorPoint = topLeftItem->GetBoundingBox().GetPosition();
-                }
-                else
-                {
-                    anchorPoint = topLeftItem->GetPosition();
-                }
-            }
-        }
+        wxPoint anchorPoint = getAnchorPoint( selection, params );
 
         wxPoint finalMoveVector = params.translation - anchorPoint;
 
@@ -802,6 +754,77 @@ int EDIT_TOOL::MoveExact( const TOOL_EVENT& aEvent )
     return 0;
 }
 
+wxPoint EDIT_TOOL::getAnchorPoint( const SELECTION &selection, const MOVE_PARAMETERS &params ) const
+{
+    wxPoint anchorPoint;
+
+    if( params.origin == RELATIVE_TO_CURRENT_POSITION )
+    {
+        return wxPoint( 0, 0 );
+    }
+
+    // set default anchor
+    VECTOR2I rp = selection.GetCenter();
+    anchorPoint = wxPoint( rp.x, rp.y );
+
+    // If the anchor is not ANCHOR_FROM_LIBRARY then the user applied an override.
+    // Also run through this block if only one item is slected because it may be a module,
+    // in which case we want something different than the center of the selection
+    if( ( params.anchor != ANCHOR_FROM_LIBRARY ) || ( selection.GetSize() == 1 ) )
+        {
+            BOARD_ITEM* topLeftItem = static_cast<BOARD_ITEM*>( selection.GetTopLeftModule() );
+
+            // no module found if the GetTopLeftModule() returns null
+            if( topLeftItem != nullptr )
+            {
+
+                if( topLeftItem->Type() == PCB_MODULE_T )
+                {
+                    // Cast to module to allow access to the pads
+                    MODULE* mod = static_cast<MODULE*>( topLeftItem );
+
+                    switch( params.anchor )
+                    {
+                    case ANCHOR_FROM_LIBRARY:
+                        anchorPoint = mod->GetPosition();
+                        break;
+                    case ANCHOR_TOP_LEFT_PAD:
+                        topLeftItem = mod->GetTopLeftPad();
+                        break;
+                    case ANCHOR_CENTER_FOOTPRINT:
+                        anchorPoint = mod->GetFootprintRect().GetCenter();
+                        break;
+                    }
+                }
+
+                if( topLeftItem->Type() == PCB_PAD_T )
+                {
+                    if( static_cast<D_PAD*>( topLeftItem )->GetAttribute() == PAD_ATTRIB_SMD )
+                    {
+                        // Use the top left corner of SMD pads as an anchor instead of the center
+                        anchorPoint = topLeftItem->GetBoundingBox().GetPosition();
+                    }
+                    else
+                    {
+                        anchorPoint = topLeftItem->GetPosition();
+                    }
+                }
+            }
+            else // no module found in the selection
+            {
+                // in a selection of non-modules
+                if( params.anchor == ANCHOR_TOP_LEFT_PAD )
+                {
+                    // approach the top left pad override for non-modules by using the position of
+                    // the topleft item as an anchor
+                    topLeftItem = static_cast<BOARD_ITEM*>( selection.GetTopLeftItem() );
+                    anchorPoint = topLeftItem->GetPosition();
+                }
+            }
+        }
+    return anchorPoint;
+}
+
 
 int EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent )
 {
diff --git a/pcbnew/tools/edit_tool.h b/pcbnew/tools/edit_tool.h
index 6ca3d7b..b71c466 100644
--- a/pcbnew/tools/edit_tool.h
+++ b/pcbnew/tools/edit_tool.h
@@ -28,6 +28,7 @@
 
 #include <math/vector2d.h>
 #include <tools/pcb_tool.h>
+#include <dialogs/dialog_move_exact.h>
 
 class BOARD_COMMIT;
 class BOARD_ITEM;
@@ -199,6 +200,8 @@ private:
     }
 
     std::unique_ptr<BOARD_COMMIT> m_commit;
+
+    wxPoint getAnchorPoint( const SELECTION &selection, const MOVE_PARAMETERS &params ) const;
 };
 
 #endif
-- 
2.7.4