kicad-developers team mailing list archive

[Maciej Sumiński <maciej.suminski@xxxxxxx>]

Hi Jon,

The current version looks much better to me. From what I see there is no
actual bounding box caching, as GetBoundingBox() always calls
ComputeBoundingBox(). I am fine with that, but before I push the patch I
need to ask: does anyone know why the board bounding box is cached? I
believe it must have been done to boost performance of certain parts of
the code, but I wonder if it is really necessary. Especially that one
needs to know that it has to be updated using ComputeBoundingBox().

Just by looking at the code, I noticed that the autorouter calls
BOARD::GetBoundingBox() frequently, but I could not see much difference
with caching disabled. Likely, the routing algorithm takes significantly
more time than the bounding box calculations.

Personally I would completely remove m_BoundingBox field instead of
making it mutable. Also, I have noticed there are a few places where the
bounding box is overridden with SetBoundingBox(). It seems to me a bit
dubious, as bounding box should depend solely on the items belonging to
the board. I attach a patch to show what I would change. Any comments,
especially from Wayne & Jean-Pierre?

Regards,
Orson

On 02/23/2017 01:49 PM, Jon Evans wrote:
> Hi Orson,
> 
> Here's an updated patch with the changes you requested.  The only issue is,
> without some kind of caching, I had to change the call sites that were
> interested in the board bounding box with edges only, so the patch has
> grown in scope.  Let me know if this looks better.
> 
> Best,
> Jon
> 
> On Thu, Feb 23, 2017 at 4:17 AM, Maciej Sumiński <maciej.suminski@xxxxxxx>
> wrote:
> 
>> Hi Jon,
>>
>> I really like the generic approach in the zoom methods. This part I
>> would merge instantly, but there is an issue with caching the board
>> bounding box. It does not take into account that items already added to
>> board may change their position and affect the bounding box. I would
>> remove caching completely, what do you think?
>>
>> If you are going to modify the patch, would you rename boardBBox in
>> COMMON_TOOLS::ZoomFitScreen() to bbox or anything that is not related to
>> board? Thank you in advance.
>>
>> Regards,
>> Orson
>>
>> On 02/23/2017 05:34 AM, Jon Evans wrote:
>>> Hi all,
>>>
>>> This patch finishes off the refactor I did a few days ago, by enabling
>>> ZoomFitScreen to work without knowledge of the BOARD class.
>>>
>>> In order to make this work, I changed the way GetBoundingBox and
>>> ComputeBoundingBox work so that the call sites of GetBoundingBox don't
>> have
>>> to also call ComputeBoundingBox if they don't need to use the
>>> aBoardEdgesOnly flag.
>>>
>>> Those with good knowledge of BOARD should review this to make sure I
>> caught
>>> all the instances where we should mark the bounding box as needing to be
>>> re-computed.
>>>
>>> Best,
>>> Jon
>>>
>>>
>>>
>>> _______________________________________________
>>> Mailing list: https://launchpad.net/~kicad-developers
>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>> More help   : https://help.launchpad.net/ListHelp
>>>
>>
>>
>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~kicad-developers
>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~kicad-developers
>> More help   : https://help.launchpad.net/ListHelp
>>
>>
> 

From 0285240d26bfdb4ca9d5f8d2e1214deb3c48cfcd Mon Sep 17 00:00:00 2001
From: Maciej Suminski <maciej.suminski@xxxxxxx>
Date: Fri, 24 Feb 2017 09:29:42 +0100
Subject: [PATCH] Remove BOARD::SetBoundingBox()

Bounding box should be computed basing on the items belonging to the
board.
---
 pcbnew/autorouter/routing_matrix.cpp |  2 --
 pcbnew/autorouter/solve.cpp          | 44 +++++++++++++++++-------------------
 pcbnew/class_board.cpp               |  2 --
 pcbnew/class_board.h                 | 12 +---------
 pcbnew/legacy_plugin.cpp             | 12 ++++------
 5 files changed, 26 insertions(+), 46 deletions(-)

diff --git a/pcbnew/autorouter/routing_matrix.cpp b/pcbnew/autorouter/routing_matrix.cpp
index 94163641e..470b3990e 100644
--- a/pcbnew/autorouter/routing_matrix.cpp
+++ b/pcbnew/autorouter/routing_matrix.cpp
@@ -86,8 +86,6 @@ bool MATRIX_ROUTING_HEAD::ComputeMatrixSize( BOARD* aPcb, bool aUseBoardEdgesOnl
 
     m_BrdBox.SetEnd( end );
 
-    aPcb->SetBoundingBox( m_BrdBox );
-
     m_Nrows = m_BrdBox.GetHeight() / m_GridRouting;
     m_Ncols = m_BrdBox.GetWidth() / m_GridRouting;
 
diff --git a/pcbnew/autorouter/solve.cpp b/pcbnew/autorouter/solve.cpp
index c10e57f8e..27966319e 100644
--- a/pcbnew/autorouter/solve.cpp
+++ b/pcbnew/autorouter/solve.cpp
@@ -51,6 +51,7 @@
 
 static int Autoroute_One_Track( PCB_EDIT_FRAME* pcbframe,
                                 wxDC*           DC,
+                                const EDA_RECT& bbox,
                                 int             two_sides,
                                 int             row_source,
                                 int             col_source,
@@ -288,6 +289,8 @@ int PCB_EDIT_FRAME::Solve( wxDC* DC, int aLayersCount )
     GetWork( &row_source, &col_source, &current_net_code,
              &row_target, &col_target, &pt_cur_ch ); // First net to route.
 
+    const EDA_RECT bbox = GetBoard()->GetBoundingBox();
+
     for( ; row_source != ILLEGAL; GetWork( &row_source, &col_source,
                                            &current_net_code, &row_target,
                                            &col_target,
@@ -323,10 +326,10 @@ int PCB_EDIT_FRAME::Solve( wxDC* DC, int aLayersCount )
             AppendMsgPanel( wxT( "Activity" ), msg, BROWN );
         }
 
-        segm_oX = GetBoard()->GetBoundingBox().GetX() + (RoutingMatrix.m_GridRouting * col_source);
-        segm_oY = GetBoard()->GetBoundingBox().GetY() + (RoutingMatrix.m_GridRouting * row_source);
-        segm_fX = GetBoard()->GetBoundingBox().GetX() + (RoutingMatrix.m_GridRouting * col_target);
-        segm_fY = GetBoard()->GetBoundingBox().GetY() + (RoutingMatrix.m_GridRouting * row_target);
+        segm_oX = bbox.GetX() + ( RoutingMatrix.m_GridRouting * col_source );
+        segm_oY = bbox.GetY() + ( RoutingMatrix.m_GridRouting * row_source );
+        segm_fX = bbox.GetX() + ( RoutingMatrix.m_GridRouting * col_target );
+        segm_fY = bbox.GetY() + ( RoutingMatrix.m_GridRouting * row_target );
 
         // Draw segment.
         GRLine( m_canvas->GetClipBox(), DC,
@@ -335,7 +338,7 @@ int PCB_EDIT_FRAME::Solve( wxDC* DC, int aLayersCount )
         pt_cur_ch->m_PadStart->Draw( m_canvas, DC, GR_OR | GR_HIGHLIGHT );
         pt_cur_ch->m_PadEnd->Draw( m_canvas, DC, GR_OR | GR_HIGHLIGHT );
 
-        success = Autoroute_One_Track( this, DC,
+        success = Autoroute_One_Track( this, DC, bbox,
                                        two_sides, row_source, col_source,
                                        row_target, col_target, pt_cur_ch );
 
@@ -398,6 +401,7 @@ int PCB_EDIT_FRAME::Solve( wxDC* DC, int aLayersCount )
  */
 static int Autoroute_One_Track( PCB_EDIT_FRAME* pcbframe,
                                 wxDC*           DC,
+                                const EDA_RECT& bbox,
                                 int             two_sides,
                                 int             row_source,
                                 int             col_source,
@@ -474,10 +478,8 @@ static int Autoroute_One_Track( PCB_EDIT_FRAME* pcbframe,
      * On the routing grid (1 grid point must be in the pad)
      */
     {
-        int cX = ( RoutingMatrix.m_GridRouting * col_source )
-                 + pcbframe->GetBoard()->GetBoundingBox().GetX();
-        int cY = ( RoutingMatrix.m_GridRouting * row_source )
-                 + pcbframe->GetBoard()->GetBoundingBox().GetY();
+        int cX = ( RoutingMatrix.m_GridRouting * col_source ) + bbox.GetX();
+        int cY = ( RoutingMatrix.m_GridRouting * row_source ) + bbox.GetY();
         int dx = pt_cur_ch->m_PadStart->GetSize().x / 2;
         int dy = pt_cur_ch->m_PadStart->GetSize().y / 2;
         int px = pt_cur_ch->m_PadStart->GetPosition().x;
@@ -489,10 +491,8 @@ static int Autoroute_One_Track( PCB_EDIT_FRAME* pcbframe,
         if( ( abs( cX - px ) > dx ) || ( abs( cY - py ) > dy ) )
             goto end_of_route;
 
-        cX = ( RoutingMatrix.m_GridRouting * col_target )
-             + pcbframe->GetBoard()->GetBoundingBox().GetX();
-        cY = ( RoutingMatrix.m_GridRouting * row_target )
-             + pcbframe->GetBoard()->GetBoundingBox().GetY();
+        cX = ( RoutingMatrix.m_GridRouting * col_target ) + bbox.GetX();
+        cY = ( RoutingMatrix.m_GridRouting * row_target ) + bbox.GetY();
         dx = pt_cur_ch->m_PadEnd->GetSize().x / 2;
         dy = pt_cur_ch->m_PadEnd->GetSize().y / 2;
         px = pt_cur_ch->m_PadEnd->GetPosition().x;
@@ -1164,19 +1164,19 @@ static int Retrace( PCB_EDIT_FRAME* pcbframe, wxDC* DC,
 static void OrCell_Trace( BOARD* pcb, int col, int row,
                           int side, int orient, int current_net_code )
 {
+    const EDA_RECT bbox = pcb->GetBoundingBox();
+
     if( orient == HOLE )  // placement of a via
     {
-        VIA *newVia = new VIA( pcb );
+        VIA* newVia = new VIA( pcb );
 
         g_CurrentTrackList.PushBack( newVia );
 
         g_CurrentTrackSegment->SetState( TRACK_AR, true );
         g_CurrentTrackSegment->SetLayer( F_Cu );
 
-        g_CurrentTrackSegment->SetStart(wxPoint( pcb->GetBoundingBox().GetX() +
-                                                ( RoutingMatrix.m_GridRouting * row ),
-                                                pcb->GetBoundingBox().GetY() +
-                                                ( RoutingMatrix.m_GridRouting * col )));
+        g_CurrentTrackSegment->SetStart( wxPoint( bbox.GetX() + RoutingMatrix.m_GridRouting * row,
+                            bbox.GetY() + RoutingMatrix.m_GridRouting * col ) );
         g_CurrentTrackSegment->SetEnd( g_CurrentTrackSegment->GetStart() );
 
         g_CurrentTrackSegment->SetWidth( pcb->GetDesignSettings().GetCurrentViaSize() );
@@ -1198,10 +1198,8 @@ static void OrCell_Trace( BOARD* pcb, int col, int row,
             g_CurrentTrackSegment->SetLayer( g_Route_Layer_TOP );
 
         g_CurrentTrackSegment->SetState( TRACK_AR, true );
-        g_CurrentTrackSegment->SetEnd( wxPoint( pcb->GetBoundingBox().GetX() +
-                                         ( RoutingMatrix.m_GridRouting * row ),
-                                         pcb->GetBoundingBox().GetY() +
-                                         ( RoutingMatrix.m_GridRouting * col )));
+        g_CurrentTrackSegment->SetEnd( wxPoint( bbox.GetX() + RoutingMatrix.m_GridRouting * row,
+                            bbox.GetY() + RoutingMatrix.m_GridRouting * col ) );
         g_CurrentTrackSegment->SetNetCode( current_net_code );
 
         if( g_CurrentTrackSegment->Back() == NULL ) // Start trace.
@@ -1344,5 +1342,5 @@ static void AddNewTrace( PCB_EDIT_FRAME* pcbframe, wxDC* DC )
 
     pcbframe->TestNetConnection( DC, netcode );
 
-    screen->SetModify();
+    screen->SetModified();
 }
diff --git a/pcbnew/class_board.cpp b/pcbnew/class_board.cpp
index f8ace58b8..f32c0bf99 100644
--- a/pcbnew/class_board.cpp
+++ b/pcbnew/class_board.cpp
@@ -1123,8 +1123,6 @@ EDA_RECT BOARD::ComputeBoundingBox( bool aBoardEdgesOnly ) const
         }
     }
 
-    m_BoundingBox = area;   // save for BOARD::GetBoundingBox()
-
     return area;
 }
 
diff --git a/pcbnew/class_board.h b/pcbnew/class_board.h
index e7a910cd7..10e893b20 100644
--- a/pcbnew/class_board.h
+++ b/pcbnew/class_board.h
@@ -185,7 +185,6 @@ private:
 
     int                     m_fileFormatVersionAtLoad;  ///< the version loaded from the file
 
-    mutable EDA_RECT        m_BoundingBox;
     NETINFO_LIST            m_NetInfo;              ///< net info list (name, design constraints ..
     RN_DATA*                m_ratsnest;
 
@@ -823,19 +822,12 @@ public:
      * calculates the bounding box containing all board items (or board edge segments).
      * @param aBoardEdgesOnly is true if we are interested in board edge segments only.
      * @return EDA_RECT - the board's bounding box
-     * @see PCB_BASE_FRAME::GetBoardBoundingBox() which calls this and doctors the result
      */
     EDA_RECT ComputeBoundingBox( bool aBoardEdgesOnly = false ) const;
 
-    /**
-     * Function GetBoundingBox
-     * may be called soon after ComputeBoundingBox() to return the same EDA_RECT,
-     * as long as the BOARD has not changed.  Remember, ComputeBoundingBox()'s
-     * aBoardEdgesOnly argument is considered in this return value also.
-     */
     const EDA_RECT GetBoundingBox() const override
     {
-        return ComputeBoundingBox();
+        return ComputeBoundingBox( false );
     }
 
     const EDA_RECT GetBoardEdgesBoundingBox() const
@@ -843,8 +835,6 @@ public:
         return ComputeBoundingBox( true );
     }
 
-    void SetBoundingBox( const EDA_RECT& aBox ) { m_BoundingBox = aBox; }
-
     void GetMsgPanelInfo( std::vector< MSG_PANEL_ITEM >& aList ) override;
 
     /**
diff --git a/pcbnew/legacy_plugin.cpp b/pcbnew/legacy_plugin.cpp
index 21026b155..68e5b4067 100644
--- a/pcbnew/legacy_plugin.cpp
+++ b/pcbnew/legacy_plugin.cpp
@@ -660,14 +660,10 @@ void LEGACY_PLUGIN::loadGENERAL()
 
         else if( TESTLINE( "Di" ) )
         {
-            BIU x1 = biuParse( line + SZ( "Di" ), &data );
-            BIU y1 = biuParse( data, &data );
-            BIU x2 = biuParse( data, &data );
-            BIU y2 = biuParse( data );
-
-            EDA_RECT bbbox( wxPoint( x1, y1 ), wxSize( x2-x1, y2-y1 ) );
-
-            m_board->SetBoundingBox( bbbox );
+            biuParse( line + SZ( "Di" ), &data );
+            biuParse( data, &data );
+            biuParse( data, &data );
+            biuParse( data );
         }
 
         /* This is no more usefull, so this info is no more parsed
-- 
2.11.0

Attachment: signature.asc
Description: OpenPGP digital signature