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 <[email protected]> > 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 : [email protected] >>> Unsubscribe : https://launchpad.net/~kicad-developers >>> More help : https://help.launchpad.net/ListHelp >>> >> >> >> >> _______________________________________________ >> Mailing list: https://launchpad.net/~kicad-developers >> Post to : [email protected] >> 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 <[email protected]> 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, ¤t_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, ¤t_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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
