kicad-developers team mailing list archive

[Jon Evans <jon@xxxxxxxxxxxxx>]

Hi Orson, Tom,

Thanks for the tips.  I eventually did figure out what was going on --
mismatch of state between the RTREE and the View in my first attempt at
optimization.

The attached 3 patches improve performance a ton on larger Gerber files --
on my computer, between 30-40% improvement, and massive improvement in file
load time and responsiveness when you select items.
Please review and if you see no issues, apply on top of the existing branch
before you merge. (yes, there is no 0003- patch)

(This should also be good for a performance boost in PcbNew when working
with very large boards)

Thanks,
Jon

On Sat, Sep 23, 2017 at 3:01 PM, jp charras <jp.charras@xxxxxxxxxx> wrote:

> Le 21/09/2017 à 14:04, Wayne Stambaugh a écrit :
> > Orson,
> >
> > I'm leaving this decision up to you and JP since I have not had time to
> > test it.  JP, do you have any other concerns about this?
>
> Sorry for the delay, but I was away last 3 days.
> I do not have concerns about merging Gerbview GAL.
>
> We know there are a few issues, but they can be fixed later. None is
> blocking.
> And the legacy canvas can be used with no issue.
>
> So, Orson, if you can commit this very good enhancement, please do it.
> AFAIK, remember also only the GAL canvas works on wxWidgets + GTK3, so the
> GAL canvas support is
> very important for the future of KiCad.
>
> Thanks to Jon and Orson for all this work.
>
> >
> > Thanks,
> >
> > Wayne
> >
> > On 9/21/2017 6:05 AM, Maciej Sumiński wrote:
> >> Hi Jon,
> >>
> >> Thanks you, this is really cool! Now it is even more tempting to merge
> >> the gerbview_gal branch. I am going to wait one more day for vetos and
> >> tomorrow I will push it to the master branch.
> >>
> >> Regards,
> >> Orson
> >>
> >> On 09/20/2017 09:57 PM, Jon Evans wrote:
> >>> Hi Orson,
> >>>
> >>> Give this a shot in your branch.  It should work in pcbnew also now.
> >>>
> >>> -Jon
> >>>
> >>> On Wed, Sep 20, 2017 at 9:28 AM, Jon Evans <jon@xxxxxxxxxxxxx> wrote:
> >>>
> >>>> Hi Orson,
> >>>>
> >>>> Thank you for staging this for merge on your branch.  I checked and
> you do
> >>>> have all the patches.
> >>>>
> >>>> 1) Yes I planned on refactoring the selection tool once things
> stabilized
> >>>> with the highlighting etc.
> >>>> 2) Do you mean when you are highlighting Gerber X2 attributes, or
> when you
> >>>> are deselecting things, or something else?
> >>>> 3) That's a good idea on VIEW_GROUP, I will give it a try and send a
> patch.
> >>>>
> >>>> Thanks,
> >>>> Jon
> >>>>
> >>>> On Wed, Sep 20, 2017 at 5:46 AM, Maciej Sumiński <
> maciej.suminski@xxxxxxx>
> >>>> wrote:
> >>>>
> >>>>> Hi Jon,
> >>>>>
> >>>>> GALifying GerbView is a huge task, so thank you very much for your
> work!
> >>>>> I have just tested your changes and in my opinion it is in a state
> that
> >>>>> deserves merging and further tests. The new way of item highlighting
> is
> >>>>> awesome, we need to port it to pcbnew as well.
> >>>>>
> >>>>> For now I keep your patches in a separate branch, with some minor
> >>>>> modifications on top of it [1]. Please verify it contains all the
> needed
> >>>>> patches. If nobody objects, I would like to merge it this week.
> >>>>>
> >>>>> Just a few minor remarks:
> >>>>> - It seems there is some code that could be refactored to share it
> with
> >>>>> pcbnew (e.g. selection tool).
> >>>>> - 'Clear highlight' operation takes long time to finish (seems more
> than
> >>>>> with the legacy canvas), but I cannot really see what is happening
> >>>>> there. If it cannot be easily fixed, perhaps it could set the mouse
> >>>>> cursor to busy.
> >>>>> - For the new highlighting method: perhaps a more universal way is to
> >>>>> create a temporary VIEW_GROUP object containing the selection
> candidate.
> >>>>> This way it can be temporarily displayed on the overlay layer,
> without
> >>>>> modifying the original ViewGetLayer() methods.
> >>>>>
> >>>>> Regards,
> >>>>> Orson
> >>>>>
> >>>>> 1. https://code.launchpad.net/~orsonmmz/kicad/+git/kicad/+ref/
> >>>>> gerbview_gal
> >>>>>
> >>>>> On 09/18/2017 12:47 AM, Jon Evans wrote:
> >>>>>> Hi all,
> >>>>>>
> >>>>>> The day has finally come!  I have distilled my GerbView GAL branch
> into
> >>>>> a
> >>>>>> patchset attached to this email.  Hopefully with this merged into
> >>>>> master we
> >>>>>> can identify any remaining bugs and clean it up for 5.0.
> >>>>>>
> >>>>>> Note that this set is split into 5 patches to make review easier,
> but
> >>>>> they
> >>>>>> are not intended to compile and work independently.
> >>>>>>
> >>>>>> Best,
> >>>>>> Jon
>
>
>
> --
> Jean-Pierre CHARRAS
>
> _______________________________________________
> 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 3ffc218745a69d6c3d9f831ab5e14095e8db56f9 Mon Sep 17 00:00:00 2001
From: Jon Evans <jon@xxxxxxxxxxxxx>
Date: Sun, 24 Sep 2017 21:22:23 -0400
Subject: [PATCH 4/4] Skip calling RTREE::Remove() when adding items to a VIEW

---
 common/view/view.cpp     | 23 ++++++++++++++++-------
 include/view/view_item.h |  3 ++-
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/common/view/view.cpp b/common/view/view.cpp
index 80536ce1a..af2633e3e 100644
--- a/common/view/view.cpp
+++ b/common/view/view.cpp
@@ -335,7 +335,7 @@ void VIEW::Add( VIEW_ITEM* aItem, int aDrawPriority )
     }
 
     SetVisible( aItem, true );
-    Update( aItem, KIGFX::ALL );
+    Update( aItem, KIGFX::INITIAL_ADD );
 }
 
 
@@ -827,7 +827,7 @@ struct VIEW::drawItem
     {
         wxASSERT( aItem->viewPrivData() );
 
-        // Conditions that have te be fulfilled for an item to be drawn
+        // Conditions that have to be fulfilled for an item to be drawn
         bool drawCondition = aItem->viewPrivData()->isRenderable() &&
                              aItem->ViewGetLOD( layer, view ) < view->m_scale;
         if( !drawCondition )
@@ -1076,14 +1076,23 @@ void VIEW::clearGroupCache()
 
 void VIEW::invalidateItem( VIEW_ITEM* aItem, int aUpdateFlags )
 {
-    // updateLayers updates geometry too, so we do not have to update both of them at the same time
-    if( aUpdateFlags & LAYERS )
+    if( aUpdateFlags & INITIAL_ADD )
     {
-        updateLayers( aItem );
+        // Don't update layers or bbox, since it was done in VIEW::Add()
+        // Now that we have initialized, set flags to ALL for the code below
+        aUpdateFlags = ALL;
     }
-    else if( aUpdateFlags & GEOMETRY )
+    else
     {
-        updateBbox( aItem );
+        // updateLayers updates geometry too, so we do not have to update both of them at the same time
+        if( aUpdateFlags & LAYERS )
+        {
+            updateLayers( aItem );
+        }
+        else if( aUpdateFlags & GEOMETRY )
+        {
+            updateBbox( aItem );
+        }
     }
 
     int layers[VIEW_MAX_LAYERS], layers_count;
diff --git a/include/view/view_item.h b/include/view/view_item.h
index bec8016bf..6ce69434e 100644
--- a/include/view/view_item.h
+++ b/include/view/view_item.h
@@ -56,7 +56,8 @@ enum VIEW_UPDATE_FLAGS {
     COLOR       = 0x02,     /// Color has changed
     GEOMETRY    = 0x04,     /// Position or shape has changed
     LAYERS      = 0x08,     /// Layers have changed
-    ALL         = 0xff
+    INITIAL_ADD = 0x10,     /// Item is being added to the view
+    ALL         = 0xef      /// All except INITIAL_ADD
 };
 
 /**
-- 
2.11.0

From 2a31cc320971bc0870f1c5574531ddcd6395ca9a Mon Sep 17 00:00:00 2001
From: Jon Evans <jon@xxxxxxxxxxxxx>
Date: Sun, 24 Sep 2017 21:12:58 -0400
Subject: [PATCH 2/4] Minor tweaks to GerbView for performance

---
 gerbview/files.cpp                |  3 +++
 gerbview/gerbview_frame.cpp       |  5 ++---
 gerbview/tools/selection_tool.cpp | 12 +++++++-----
 gerbview/tools/selection_tool.h   |  4 ++--
 4 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/gerbview/files.cpp b/gerbview/files.cpp
index 3634ac705..f3de9a57b 100644
--- a/gerbview/files.cpp
+++ b/gerbview/files.cpp
@@ -220,6 +220,9 @@ bool GERBVIEW_FRAME::LoadGerberFiles( const wxString& aFullFileName )
         m_mruPath = currentPath;
     }
 
+    // Set the busy cursor
+    wxBusyCursor wait;
+
     // Read gerber files: each file is loaded on a new GerbView layer
     bool success = true;
     int layer = GetActiveLayer();
diff --git a/gerbview/gerbview_frame.cpp b/gerbview/gerbview_frame.cpp
index c28893b93..d938237e0 100644
--- a/gerbview/gerbview_frame.cpp
+++ b/gerbview/gerbview_frame.cpp
@@ -820,12 +820,11 @@ void GERBVIEW_FRAME::SetActiveLayer( int aLayer, bool doLayerWidgetUpdate )
         m_toolManager->RunAction( GERBVIEW_ACTIONS::layerChanged );       // notify other tools
         GetGalCanvas()->SetFocus();                 // otherwise hotkeys are stuck somewhere
 
+        // NOTE(JE) The next two calls are slow (scales with number of items)
         GetGalCanvas()->SetTopLayer( GERBER_DRAW_LAYER( aLayer ) );
-
         GetGalCanvas()->SetHighContrastLayer( GERBER_DRAW_LAYER( aLayer ) );
-        GetGalCanvas()->Refresh();
 
-        GetGalCanvas()->GetView()->RecacheAllItems();
+        GetGalCanvas()->Refresh();
     }
 }
 
diff --git a/gerbview/tools/selection_tool.cpp b/gerbview/tools/selection_tool.cpp
index d12fd893c..f094f12c1 100644
--- a/gerbview/tools/selection_tool.cpp
+++ b/gerbview/tools/selection_tool.cpp
@@ -704,7 +704,6 @@ EDA_ITEM* GERBVIEW_SELECTION_TOOL::disambiguationMenu( GERBER_COLLECTOR* aCollec
 
     getView()->Remove( &highlightGroup );
 
-
     return current;
 }
 
@@ -732,9 +731,9 @@ void GERBVIEW_SELECTION_TOOL::select( EDA_ITEM* aItem )
         return;
     }
 
-    selectVisually( aItem );
     m_selection.Add( aItem );
     getView()->Add( &m_selection );
+    selectVisually( aItem );
 
     if( m_selection.Size() == 1 )
     {
@@ -765,7 +764,7 @@ void GERBVIEW_SELECTION_TOOL::unselect( EDA_ITEM* aItem )
 }
 
 
-void GERBVIEW_SELECTION_TOOL::selectVisually( EDA_ITEM* aItem ) const
+void GERBVIEW_SELECTION_TOOL::selectVisually( EDA_ITEM* aItem )
 {
     // Move the item's layer to the front
     int layer = static_cast<GERBER_DRAW_ITEM*>( aItem )->GetLayer();
@@ -774,16 +773,19 @@ void GERBVIEW_SELECTION_TOOL::selectVisually( EDA_ITEM* aItem ) const
     // Hide the original item, so it is shown only on overlay
     aItem->SetSelected();
     getView()->Hide( aItem, true );
-    getView()->Update( aItem, KIGFX::GEOMETRY );
+
+    getView()->Update( &m_selection );
 }
 
 
-void GERBVIEW_SELECTION_TOOL::unselectVisually( EDA_ITEM* aItem ) const
+void GERBVIEW_SELECTION_TOOL::unselectVisually( EDA_ITEM* aItem )
 {
     // Restore original item visibility
     aItem->ClearSelected();
     getView()->Hide( aItem, false );
     getView()->Update( aItem, KIGFX::ALL );
+
+    getView()->Update( &m_selection );
 }
 
 
diff --git a/gerbview/tools/selection_tool.h b/gerbview/tools/selection_tool.h
index dc0956f96..06a693a57 100644
--- a/gerbview/tools/selection_tool.h
+++ b/gerbview/tools/selection_tool.h
@@ -203,14 +203,14 @@ private:
      * Marks item as selected, but does not add it to the ITEMS_PICKED_LIST.
      * @param aItem is an item to be be marked.
      */
-    void selectVisually( EDA_ITEM* aItem ) const;
+    void selectVisually( EDA_ITEM* aItem );
 
     /**
      * Function unselectVisually()
      * Marks item as selected, but does not add it to the ITEMS_PICKED_LIST.
      * @param aItem is an item to be be marked.
      */
-    void unselectVisually( EDA_ITEM* aItem ) const;
+    void unselectVisually( EDA_ITEM* aItem );
 
     /**
      * Function selectionContains()
-- 
2.11.0

From 6be8ebf1c0dd7579745a7f68b0b51d6c7ff79484 Mon Sep 17 00:00:00 2001
From: Jon Evans <jon@xxxxxxxxxxxxx>
Date: Sun, 24 Sep 2017 21:06:33 -0400
Subject: [PATCH 1/4] Use unordered_map instead of map for GROUPS_MAP

---
 include/gal/opengl/opengl_gal.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/gal/opengl/opengl_gal.h b/include/gal/opengl/opengl_gal.h
index 928d21082..97ddadc69 100644
--- a/include/gal/opengl/opengl_gal.h
+++ b/include/gal/opengl/opengl_gal.h
@@ -41,7 +41,7 @@
 
 #include <wx/glcanvas.h>
 
-#include <map>
+#include <unordered_map>
 #include <boost/smart_ptr/shared_array.hpp>
 #include <memory>
 
@@ -299,7 +299,7 @@ private:
     static GLuint fontTexture;                  ///< Bitmap font texture handle (shared)
 
     // Vertex buffer objects related fields
-    typedef std::map< unsigned int, std::shared_ptr<VERTEX_ITEM> > GROUPS_MAP;
+    typedef std::unordered_map< unsigned int, std::shared_ptr<VERTEX_ITEM> > GROUPS_MAP;
     GROUPS_MAP              groups;                 ///< Stores informations about VBO objects (groups)
     unsigned int            groupCounter;           ///< Counter used for generating keys for groups
     VERTEX_MANAGER*         currentManager;         ///< Currently used VERTEX_MANAGER (for storing VERTEX_ITEMs)
-- 
2.11.0