← Back to team overview

kicad-developers team mailing list archive

[PATCH] pcbnew alignment bugfixes

 

​Attached are 4 patches for various alignment issues.

The first fixes https://bugs.launchpad.net/kicad/+bug/1751352 where pads
are moved despite being locked.

The second is a bugfix where aligning in the X direction moved things in
the Y direction.

The third is a bugfix for when you select both a pad and the pad's parent
module before aligning.

The fourth is not a bugfix and, as such should be considered highly
optional at this point.  But it fixes an annoyance when aligning a group of
things to their centers.  I can never tell which item will be chosen for
the center as currently it chooses the "centermost" item.  If you have
10-20 mostly-aligned items, it's very hard to tell which will be the
center.  You need to move about half to one side and about half to the
other before aligning.  Or align, check the value and do a second move.
Instead, this chooses the left-most for X-center and top-most for
Y-center.  You can then move one item into place and align the others to
it.  Delaying this has no affect of the other patches, so I'm happy to hold
it for post-5.​
From c861f742e42a1cdb3dccf3a6668a375d3b2c4c25 Mon Sep 17 00:00:00 2001
From: Seth Hillbrand <hillbrand@xxxxxxxxxxx>
Date: Mon, 26 Feb 2018 15:13:21 -0800
Subject: [PATCH 1/4] pcbnew: Check locks in alignment

When aligning in pcbnew, check for pad/module locks before performing a
move and query the user.

When aligning on pads, don't move the pad without moving the footprint,
so we don't break footprints.

Fixes: lp:1751352
* https://bugs.launchpad.net/kicad/+bug/1751352
---
 pcbnew/tools/placement_tool.cpp | 184 ++++++++++++++++++++++++++++++++--------
 pcbnew/tools/placement_tool.h   |  10 +++
 2 files changed, 160 insertions(+), 34 deletions(-)

diff --git a/pcbnew/tools/placement_tool.cpp b/pcbnew/tools/placement_tool.cpp
index f0ae369ca..d5589fba0 100644
--- a/pcbnew/tools/placement_tool.cpp
+++ b/pcbnew/tools/placement_tool.cpp
@@ -182,19 +182,69 @@ ALIGNMENT_RECTS GetBoundingBoxes( const SELECTION &sel )
 }
 
 
+int ALIGN_DISTRIBUTE_TOOL::checkLockedStatus( const SELECTION &selection ) const
+{
+    SELECTION moving_items( selection );
+
+    // Remove the anchor from the list
+    moving_items.Remove( moving_items.Front() );
+
+    bool containsLocked = false;
+
+    // Check if the selection contains locked items
+    for( const auto& item : moving_items )
+    {
+        switch ( item->Type() )
+        {
+        case PCB_MODULE_T:
+            if( static_cast< MODULE* >( item )->IsLocked() )
+                containsLocked = true;
+            break;
+
+        case PCB_PAD_T:
+        case PCB_MODULE_EDGE_T:
+        case PCB_MODULE_TEXT_T:
+            if( static_cast< MODULE* >( item->GetParent() )->IsLocked() )
+                containsLocked = true;
+            break;
+
+        default:    // suppress warnings
+            break;
+        }
+    }
+
+    if( containsLocked )
+    {
+        if( IsOK( getEditFrame< PCB_EDIT_FRAME >(),
+                _( "Selection contains locked items. Do you want to continue?" ) ) )
+        {
+            return SELECTION_LOCK_OVERRIDE;
+        }
+        else
+            return SELECTION_LOCKED;
+    }
+
+    return SELECTION_UNLOCKED;
+}
+
+
 int ALIGN_DISTRIBUTE_TOOL::AlignTop( const TOOL_EVENT& aEvent )
 {
-    const SELECTION& selection = m_selectionTool->GetSelection();
+    auto frame = getEditFrame<PCB_BASE_FRAME>();
+    const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
     if( selection.Size() <= 1 )
         return 0;
 
-    BOARD_COMMIT commit( getEditFrame<PCB_BASE_FRAME>() );
-    commit.StageItems( selection, CHT_MODIFY );
-
     auto itemsToAlign = GetBoundingBoxes( selection );
     std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortTopmostY );
 
+    if( checkLockedStatus( selection ) == SELECTION_LOCKED )
+        return 0;
+
+    BOARD_COMMIT commit( frame );
+    commit.StageItems( selection, CHT_MODIFY );
+
     // after sorting, the fist item acts as the target for all others
     const int targetTop = itemsToAlign.begin()->second.GetY();
 
@@ -202,7 +252,13 @@ int ALIGN_DISTRIBUTE_TOOL::AlignTop( const TOOL_EVENT& aEvent )
     for( auto& i : itemsToAlign )
     {
         int difference = targetTop - i.second.GetY();
-        i.first->Move( wxPoint( 0, difference ) );
+        BOARD_ITEM* item = i.first;
+
+        // Don't move a pad by itself unless editing the footprint
+        if( item->Type() == PCB_PAD_T && frame->IsType( FRAME_PCB ) )
+            item = item->GetParent();
+
+        item->Move( wxPoint( 0, difference ) );
     }
 
     commit.Push( _( "Align to top" ) );
@@ -213,17 +269,21 @@ int ALIGN_DISTRIBUTE_TOOL::AlignTop( const TOOL_EVENT& aEvent )
 
 int ALIGN_DISTRIBUTE_TOOL::AlignBottom( const TOOL_EVENT& aEvent )
 {
-    const SELECTION& selection = m_selectionTool->GetSelection();
+    auto frame = getEditFrame<PCB_BASE_FRAME>();
+    const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
     if( selection.Size() <= 1 )
         return 0;
 
-    BOARD_COMMIT commit( getEditFrame<PCB_BASE_FRAME>() );
-    commit.StageItems( selection, CHT_MODIFY );
-
     auto itemsToAlign = GetBoundingBoxes( selection );
     std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortBottommostY );
 
+    if( checkLockedStatus( selection ) == SELECTION_LOCKED )
+        return 0;
+
+    BOARD_COMMIT commit( frame );
+    commit.StageItems( selection, CHT_MODIFY );
+
     // after sorting, the fist item acts as the target for all others
    const int targetBottom = itemsToAlign.begin()->second.GetBottom();
 
@@ -231,7 +291,13 @@ int ALIGN_DISTRIBUTE_TOOL::AlignBottom( const TOOL_EVENT& aEvent )
     for( auto& i : itemsToAlign )
     {
         int difference = targetBottom - i.second.GetBottom();
-        i.first->Move( wxPoint( 0, difference ) );
+        BOARD_ITEM* item = i.first;
+
+        // Don't move a pad by itself unless editing the footprint
+        if( item->Type() == PCB_PAD_T && frame->IsType( FRAME_PCB ) )
+            item = item->GetParent();
+
+        item->Move( wxPoint( 0, difference ) );
     }
 
     commit.Push( _( "Align to bottom" ) );
@@ -257,17 +323,21 @@ int ALIGN_DISTRIBUTE_TOOL::AlignLeft( const TOOL_EVENT& aEvent )
 
 int ALIGN_DISTRIBUTE_TOOL::doAlignLeft()
 {
-    const SELECTION& selection = m_selectionTool->GetSelection();
+    auto frame = getEditFrame<PCB_BASE_FRAME>();
+    const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
     if( selection.Size() <= 1 )
         return 0;
 
-    BOARD_COMMIT commit( getEditFrame<PCB_BASE_FRAME>() );
-    commit.StageItems( selection, CHT_MODIFY );
-
     auto itemsToAlign = GetBoundingBoxes( selection );
     std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortLeftmostX );
 
+    if( checkLockedStatus( selection ) == SELECTION_LOCKED )
+        return 0;
+
+    BOARD_COMMIT commit( frame );
+    commit.StageItems( selection, CHT_MODIFY );
+
     // after sorting, the fist item acts as the target for all others
     const int targetLeft = itemsToAlign.begin()->second.GetX();
 
@@ -275,7 +345,13 @@ int ALIGN_DISTRIBUTE_TOOL::doAlignLeft()
     for( auto& i : itemsToAlign )
     {
         int difference = targetLeft - i.second.GetX();
-        i.first->Move( wxPoint( difference, 0 ) );
+        BOARD_ITEM* item = i.first;
+
+        // Don't move a pad by itself unless editing the footprint
+        if( item->Type() == PCB_PAD_T && frame->IsType( FRAME_PCB ) )
+            item = item->GetParent();
+
+        item->Move( wxPoint( 0, difference ) );
     }
 
     commit.Push( _( "Align to left" ) );
@@ -301,17 +377,21 @@ int ALIGN_DISTRIBUTE_TOOL::AlignRight( const TOOL_EVENT& aEvent )
 
 int ALIGN_DISTRIBUTE_TOOL::doAlignRight()
 {
-    const SELECTION& selection = m_selectionTool->GetSelection();
+    auto frame = getEditFrame<PCB_BASE_FRAME>();
+    const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
     if( selection.Size() <= 1 )
         return 0;
 
-    BOARD_COMMIT commit( getEditFrame<PCB_BASE_FRAME>() );
-    commit.StageItems( selection, CHT_MODIFY );
-
     auto itemsToAlign = GetBoundingBoxes( selection );
     std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortRightmostX );
 
+    if( checkLockedStatus( selection ) == SELECTION_LOCKED )
+        return 0;
+
+    BOARD_COMMIT commit( frame );
+    commit.StageItems( selection, CHT_MODIFY );
+
     // after sorting, the fist item acts as the target for all others
     const int targetRight = itemsToAlign.begin()->second.GetRight();
 
@@ -319,7 +399,13 @@ int ALIGN_DISTRIBUTE_TOOL::doAlignRight()
     for( auto& i : itemsToAlign )
     {
         int difference = targetRight - i.second.GetRight();
-        i.first->Move( wxPoint( difference, 0 ) );
+        BOARD_ITEM* item = i.first;
+
+        // Don't move a pad by itself unless editing the footprint
+        if( item->Type() == PCB_PAD_T && frame->IsType( FRAME_PCB ) )
+            item = item->GetParent();
+
+        item->Move( wxPoint( 0, difference ) );
     }
 
     commit.Push( _( "Align to right" ) );
@@ -330,17 +416,21 @@ int ALIGN_DISTRIBUTE_TOOL::doAlignRight()
 
 int ALIGN_DISTRIBUTE_TOOL::AlignCenterX( const TOOL_EVENT& aEvent )
 {
-    const SELECTION& selection = m_selectionTool->GetSelection();
+    auto frame = getEditFrame<PCB_BASE_FRAME>();
+    const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
     if( selection.Size() <= 1 )
         return 0;
 
-    BOARD_COMMIT commit( getEditFrame<PCB_BASE_FRAME>() );
-    commit.StageItems( selection, CHT_MODIFY );
-
     auto itemsToAlign = GetBoundingBoxes( selection );
     std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortCenterX );
 
+    if( checkLockedStatus( selection ) == SELECTION_LOCKED )
+        return 0;
+
+    BOARD_COMMIT commit( frame );
+    commit.StageItems( selection, CHT_MODIFY );
+
     // after sorting use the x coordinate of the middle item as a target for all other items
     const int targetX = itemsToAlign.at( itemsToAlign.size() / 2 ).second.GetCenter().x;
 
@@ -348,7 +438,13 @@ int ALIGN_DISTRIBUTE_TOOL::AlignCenterX( const TOOL_EVENT& aEvent )
     for( auto& i : itemsToAlign )
     {
         int difference = targetX - i.second.GetCenter().x;
-        i.first->Move( wxPoint( difference, 0 ) );
+        BOARD_ITEM* item = i.first;
+
+        // Don't move a pad by itself unless editing the footprint
+        if( item->Type() == PCB_PAD_T && frame->IsType( FRAME_PCB ) )
+            item = item->GetParent();
+
+        item->Move( wxPoint( 0, difference ) );
     }
 
     commit.Push( _( "Align to middle" ) );
@@ -359,17 +455,21 @@ int ALIGN_DISTRIBUTE_TOOL::AlignCenterX( const TOOL_EVENT& aEvent )
 
 int ALIGN_DISTRIBUTE_TOOL::AlignCenterY( const TOOL_EVENT& aEvent )
 {
-    const SELECTION& selection = m_selectionTool->GetSelection();
+    auto frame = getEditFrame<PCB_BASE_FRAME>();
+    const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
     if( selection.Size() <= 1 )
         return 0;
 
-    BOARD_COMMIT commit( getEditFrame<PCB_BASE_FRAME>() );
-    commit.StageItems( selection, CHT_MODIFY );
-
     auto itemsToAlign = GetBoundingBoxes( selection );
     std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortCenterY );
 
+    if( checkLockedStatus( selection ) == SELECTION_LOCKED )
+        return 0;
+
+    BOARD_COMMIT commit( frame );
+    commit.StageItems( selection, CHT_MODIFY );
+
     // after sorting use the y coordinate of the middle item as a target for all other items
     const int targetY = itemsToAlign.at( itemsToAlign.size() / 2 ).second.GetCenter().y;
 
@@ -377,7 +477,13 @@ int ALIGN_DISTRIBUTE_TOOL::AlignCenterY( const TOOL_EVENT& aEvent )
     for( auto& i : itemsToAlign )
     {
         int difference = targetY - i.second.GetCenter().y;
-        i.first->Move( wxPoint( 0, difference ) );
+        BOARD_ITEM* item = i.first;
+
+        // Don't move a pad by itself unless editing the footprint
+        if( item->Type() == PCB_PAD_T && frame->IsType( FRAME_PCB ) )
+            item = item->GetParent();
+
+        item->Move( wxPoint( 0, difference ) );
     }
 
     commit.Push( _( "Align to center" ) );
@@ -388,12 +494,17 @@ int ALIGN_DISTRIBUTE_TOOL::AlignCenterY( const TOOL_EVENT& aEvent )
 
 int ALIGN_DISTRIBUTE_TOOL::DistributeHorizontally( const TOOL_EVENT& aEvent )
 {
-    const SELECTION& selection = m_selectionTool->GetSelection();
+    auto frame = getEditFrame<PCB_BASE_FRAME>();
+    const SELECTION& selection = m_selectionTool->RequestSelection(
+            SELECTION_EDITABLE | SELECTION_SANITIZE_PADS );
 
     if( selection.Size() <= 1 )
         return 0;
 
-    BOARD_COMMIT commit( getEditFrame<PCB_BASE_FRAME>() );
+    if( m_selectionTool->CheckLock() == SELECTION_LOCKED )
+        return 0;
+
+    BOARD_COMMIT commit( frame );
     commit.StageItems( selection, CHT_MODIFY );
 
     auto itemsToDistribute = GetBoundingBoxes( selection );
@@ -470,12 +581,17 @@ void ALIGN_DISTRIBUTE_TOOL::doDistributeCentersHorizontally( ALIGNMENT_RECTS &it
 
 int ALIGN_DISTRIBUTE_TOOL::DistributeVertically( const TOOL_EVENT& aEvent )
 {
-    const SELECTION& selection = m_selectionTool->GetSelection();
+    auto frame = getEditFrame<PCB_BASE_FRAME>();
+    const SELECTION& selection = m_selectionTool->RequestSelection(
+            SELECTION_EDITABLE | SELECTION_SANITIZE_PADS );
 
     if( selection.Size() <= 1 )
         return 0;
 
-    BOARD_COMMIT commit( getEditFrame<PCB_BASE_FRAME>() );
+    if( m_selectionTool->CheckLock() == SELECTION_LOCKED )
+        return 0;
+
+    BOARD_COMMIT commit( frame );
     commit.StageItems( selection, CHT_MODIFY );
 
     auto itemsToDistribute = GetBoundingBoxes( selection );
diff --git a/pcbnew/tools/placement_tool.h b/pcbnew/tools/placement_tool.h
index a5206ed54..3d4752029 100644
--- a/pcbnew/tools/placement_tool.h
+++ b/pcbnew/tools/placement_tool.h
@@ -120,6 +120,16 @@ private:
     CONTEXT_MENU* m_placementMenu;
 
     /**
+     * Check a selection to ensure locks are valid for alignment.
+     *
+     * This is slightly different from the standard lock checking in that we ignore the lock
+     * of the first element in the selection as this is meant to be our anchor.
+     * We also check the lock of a pad's parent as we will not move pads independently of
+     * the parent module
+     */
+    int checkLockedStatus( const SELECTION &selection ) const;
+
+    /**
      * Distributes selected items using an even spacing between the centers of their bounding boxes
      *
      * NOTE: Using the centers of bounding box of items can give unsatisfactory visual results since
-- 
2.11.0

From 8d403e3f52a31ad604ce0d4a7fc351ef77cf115d Mon Sep 17 00:00:00 2001
From: Seth Hillbrand <hillbrand@xxxxxxxxxxx>
Date: Mon, 26 Feb 2018 15:17:54 -0800
Subject: [PATCH 2/4] pcbnew: fix bug in alignment x/y code

Correctly move items in x when aligning in the x dimension
---
 pcbnew/tools/placement_tool.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/pcbnew/tools/placement_tool.cpp b/pcbnew/tools/placement_tool.cpp
index d5589fba0..0c67e865b 100644
--- a/pcbnew/tools/placement_tool.cpp
+++ b/pcbnew/tools/placement_tool.cpp
@@ -351,7 +351,7 @@ int ALIGN_DISTRIBUTE_TOOL::doAlignLeft()
         if( item->Type() == PCB_PAD_T && frame->IsType( FRAME_PCB ) )
             item = item->GetParent();
 
-        item->Move( wxPoint( 0, difference ) );
+        item->Move( wxPoint( difference, 0 ) );
     }
 
     commit.Push( _( "Align to left" ) );
@@ -405,7 +405,7 @@ int ALIGN_DISTRIBUTE_TOOL::doAlignRight()
         if( item->Type() == PCB_PAD_T && frame->IsType( FRAME_PCB ) )
             item = item->GetParent();
 
-        item->Move( wxPoint( 0, difference ) );
+        item->Move( wxPoint( difference, 0 ) );
     }
 
     commit.Push( _( "Align to right" ) );
@@ -444,7 +444,7 @@ int ALIGN_DISTRIBUTE_TOOL::AlignCenterX( const TOOL_EVENT& aEvent )
         if( item->Type() == PCB_PAD_T && frame->IsType( FRAME_PCB ) )
             item = item->GetParent();
 
-        item->Move( wxPoint( 0, difference ) );
+        item->Move( wxPoint( difference, 0 ) );
     }
 
     commit.Push( _( "Align to middle" ) );
-- 
2.11.0

From 4adc179b61307784c1778231789ba20fd8aeac67 Mon Sep 17 00:00:00 2001
From: Seth Hillbrand <hillbrand@xxxxxxxxxxx>
Date: Tue, 27 Feb 2018 16:01:47 -0800
Subject: [PATCH 3/4] pcbnew: Prevent alignment on pads + parents

Filter a selection that contains pads and the pads' parent modules
before performing alignment operations.
---
 pcbnew/tools/placement_tool.cpp | 55 ++++++++++++++++++++++++++++++-----------
 pcbnew/tools/placement_tool.h   |  5 ++++
 2 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/pcbnew/tools/placement_tool.cpp b/pcbnew/tools/placement_tool.cpp
index 0c67e865b..54fd089a3 100644
--- a/pcbnew/tools/placement_tool.cpp
+++ b/pcbnew/tools/placement_tool.cpp
@@ -182,6 +182,27 @@ ALIGNMENT_RECTS GetBoundingBoxes( const SELECTION &sel )
 }
 
 
+void ALIGN_DISTRIBUTE_TOOL::filterPadsWithModules( SELECTION &selection )
+{
+    std::set<BOARD_ITEM*> rejected;
+    for( auto i : selection )
+    {
+        auto item = static_cast<BOARD_ITEM*>( i );
+        if( item->Type() == PCB_PAD_T )
+        {
+            MODULE* mod = static_cast<MODULE*>( item->GetParent() );
+
+            // selection contains both the module and its pads - remove the pads
+            if( mod && selection.Contains( mod ) )
+                rejected.insert( item );
+        }
+    }
+
+    for( BOARD_ITEM* item : rejected )
+        selection.Remove( item );
+}
+
+
 int ALIGN_DISTRIBUTE_TOOL::checkLockedStatus( const SELECTION &selection ) const
 {
     SELECTION moving_items( selection );
@@ -231,14 +252,15 @@ int ALIGN_DISTRIBUTE_TOOL::checkLockedStatus( const SELECTION &selection ) const
 int ALIGN_DISTRIBUTE_TOOL::AlignTop( const TOOL_EVENT& aEvent )
 {
     auto frame = getEditFrame<PCB_BASE_FRAME>();
-    const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
+    SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
     if( selection.Size() <= 1 )
         return 0;
 
+    filterPadsWithModules( selection );
+
     auto itemsToAlign = GetBoundingBoxes( selection );
     std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortTopmostY );
-
     if( checkLockedStatus( selection ) == SELECTION_LOCKED )
         return 0;
 
@@ -270,14 +292,15 @@ int ALIGN_DISTRIBUTE_TOOL::AlignTop( const TOOL_EVENT& aEvent )
 int ALIGN_DISTRIBUTE_TOOL::AlignBottom( const TOOL_EVENT& aEvent )
 {
     auto frame = getEditFrame<PCB_BASE_FRAME>();
-    const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
+    SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
     if( selection.Size() <= 1 )
         return 0;
 
+    filterPadsWithModules( selection );
+
     auto itemsToAlign = GetBoundingBoxes( selection );
     std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortBottommostY );
-
     if( checkLockedStatus( selection ) == SELECTION_LOCKED )
         return 0;
 
@@ -324,14 +347,15 @@ int ALIGN_DISTRIBUTE_TOOL::AlignLeft( const TOOL_EVENT& aEvent )
 int ALIGN_DISTRIBUTE_TOOL::doAlignLeft()
 {
     auto frame = getEditFrame<PCB_BASE_FRAME>();
-    const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
+    SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
     if( selection.Size() <= 1 )
         return 0;
 
+    filterPadsWithModules( selection );
+
     auto itemsToAlign = GetBoundingBoxes( selection );
     std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortLeftmostX );
-
     if( checkLockedStatus( selection ) == SELECTION_LOCKED )
         return 0;
 
@@ -378,14 +402,15 @@ int ALIGN_DISTRIBUTE_TOOL::AlignRight( const TOOL_EVENT& aEvent )
 int ALIGN_DISTRIBUTE_TOOL::doAlignRight()
 {
     auto frame = getEditFrame<PCB_BASE_FRAME>();
-    const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
+    SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
     if( selection.Size() <= 1 )
         return 0;
 
+    filterPadsWithModules( selection );
+
     auto itemsToAlign = GetBoundingBoxes( selection );
     std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortRightmostX );
-
     if( checkLockedStatus( selection ) == SELECTION_LOCKED )
         return 0;
 
@@ -417,14 +442,15 @@ int ALIGN_DISTRIBUTE_TOOL::doAlignRight()
 int ALIGN_DISTRIBUTE_TOOL::AlignCenterX( const TOOL_EVENT& aEvent )
 {
     auto frame = getEditFrame<PCB_BASE_FRAME>();
-    const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
+    SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
     if( selection.Size() <= 1 )
         return 0;
 
+    filterPadsWithModules( selection );
+
     auto itemsToAlign = GetBoundingBoxes( selection );
     std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortCenterX );
-
     if( checkLockedStatus( selection ) == SELECTION_LOCKED )
         return 0;
 
@@ -456,14 +482,15 @@ int ALIGN_DISTRIBUTE_TOOL::AlignCenterX( const TOOL_EVENT& aEvent )
 int ALIGN_DISTRIBUTE_TOOL::AlignCenterY( const TOOL_EVENT& aEvent )
 {
     auto frame = getEditFrame<PCB_BASE_FRAME>();
-    const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
+    SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
     if( selection.Size() <= 1 )
         return 0;
 
+    filterPadsWithModules( selection );
+
     auto itemsToAlign = GetBoundingBoxes( selection );
     std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortCenterY );
-
     if( checkLockedStatus( selection ) == SELECTION_LOCKED )
         return 0;
 
@@ -495,7 +522,7 @@ int ALIGN_DISTRIBUTE_TOOL::AlignCenterY( const TOOL_EVENT& aEvent )
 int ALIGN_DISTRIBUTE_TOOL::DistributeHorizontally( const TOOL_EVENT& aEvent )
 {
     auto frame = getEditFrame<PCB_BASE_FRAME>();
-    const SELECTION& selection = m_selectionTool->RequestSelection(
+    SELECTION& selection = m_selectionTool->RequestSelection(
             SELECTION_EDITABLE | SELECTION_SANITIZE_PADS );
 
     if( selection.Size() <= 1 )
@@ -582,7 +609,7 @@ void ALIGN_DISTRIBUTE_TOOL::doDistributeCentersHorizontally( ALIGNMENT_RECTS &it
 int ALIGN_DISTRIBUTE_TOOL::DistributeVertically( const TOOL_EVENT& aEvent )
 {
     auto frame = getEditFrame<PCB_BASE_FRAME>();
-    const SELECTION& selection = m_selectionTool->RequestSelection(
+    SELECTION& selection = m_selectionTool->RequestSelection(
             SELECTION_EDITABLE | SELECTION_SANITIZE_PADS );
 
     if( selection.Size() <= 1 )
diff --git a/pcbnew/tools/placement_tool.h b/pcbnew/tools/placement_tool.h
index 3d4752029..40fc28b9e 100644
--- a/pcbnew/tools/placement_tool.h
+++ b/pcbnew/tools/placement_tool.h
@@ -120,6 +120,11 @@ private:
     CONTEXT_MENU* m_placementMenu;
 
     /**
+     * Remove pads from a multi-unit select that also includes the pads' parents
+     */
+    void filterPadsWithModules( SELECTION &selection );
+
+    /**
      * Check a selection to ensure locks are valid for alignment.
      *
      * This is slightly different from the standard lock checking in that we ignore the lock
-- 
2.11.0

From 13d5785f58e5b57fcf6ed9bfd13515d9ace18d11 Mon Sep 17 00:00:00 2001
From: Seth Hillbrand <hillbrand@xxxxxxxxxxx>
Date: Wed, 28 Feb 2018 16:34:48 -0800
Subject: [PATCH 4/4] pcbnew: align centers to the top and left

When aligning module centers, it can be hard to determine which item
will be chosen for the alignment target when it chooses based on the
median of the center values.  Instead, this patch chooses the alignment
target as the top and left most items of the Y and X centering,
respectively
---
 pcbnew/tools/placement_tool.cpp | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/pcbnew/tools/placement_tool.cpp b/pcbnew/tools/placement_tool.cpp
index 54fd089a3..c4c742e12 100644
--- a/pcbnew/tools/placement_tool.cpp
+++ b/pcbnew/tools/placement_tool.cpp
@@ -457,8 +457,9 @@ int ALIGN_DISTRIBUTE_TOOL::AlignCenterX( const TOOL_EVENT& aEvent )
     BOARD_COMMIT commit( frame );
     commit.StageItems( selection, CHT_MODIFY );
 
-    // after sorting use the x coordinate of the middle item as a target for all other items
-    const int targetX = itemsToAlign.at( itemsToAlign.size() / 2 ).second.GetCenter().x;
+    // after sorting use the center x coordinate of the leftmost item as a target
+    // for all other items
+    const int targetX = itemsToAlign.begin()->second.GetCenter().x;
 
     // Move the selected items
     for( auto& i : itemsToAlign )
@@ -497,8 +498,9 @@ int ALIGN_DISTRIBUTE_TOOL::AlignCenterY( const TOOL_EVENT& aEvent )
     BOARD_COMMIT commit( frame );
     commit.StageItems( selection, CHT_MODIFY );
 
-    // after sorting use the y coordinate of the middle item as a target for all other items
-    const int targetY = itemsToAlign.at( itemsToAlign.size() / 2 ).second.GetCenter().y;
+    // after sorting use the center y coordinate of the top-most item as a target
+    // for all other items
+    const int targetY = itemsToAlign.begin()->second.GetCenter().y;
 
     // Move the selected items
     for( auto& i : itemsToAlign )
-- 
2.11.0


Follow ups