Implement progress reporter for cvpcb footprint loading.

Fixes: lp:1676910
* https://bugs.launchpad.net/kicad/+bug/1676910

common/widgets/progress_reporter.cpp

@@ -37,37 +37,34 @@ PROGRESS_REPORTER::PROGRESS_REPORTER( int aNumPhases ) :
 
 void PROGRESS_REPORTER::BeginPhase( int aPhase )
 {
-    m_phase = aPhase;
+    m_phase.store( aPhase );
-    m_progress = 0;
+    m_progress.store( 0 );
-    updateUI();
 }
 
 
 void PROGRESS_REPORTER::AdvancePhase( )
 {
-    m_phase++;
+    m_phase.fetch_add( 1 );
-    m_progress = 0;
+    m_progress.store( 0 );
-    updateUI();
 }
 
 
 void PROGRESS_REPORTER::Report( const wxString& aMessage )
 {
+    std::lock_guard<std::mutex> guard( m_mutex );
     m_rptMessage = aMessage;
-    updateUI();
 }
 
 
 void PROGRESS_REPORTER::SetMaxProgress( int aMaxProgress )
 {
-    m_maxProgress = aMaxProgress;
+    m_maxProgress.store( aMaxProgress );
-    updateUI();
 }
 
 
 void PROGRESS_REPORTER::AdvanceProgress()
 {
-    m_progress++;
+    m_progress.fetch_add( 1 );
 }
 
 
@@ -80,14 +77,38 @@ int PROGRESS_REPORTER::currentProgress() const
 }
 
 
-// Please, *DO NOT* use wxPD_APP_MODAL style: it is not necessary
+bool PROGRESS_REPORTER::KeepRefreshing( bool aWait )
-// (without this option the PROGRESS_REPORTER is modal for the parent frame)
+{
-// and PROGRESS_REPORTER works fine on OSX only without this style
+    if( aWait )
-// when called from a quasi modal dialog
+    {
-WX_PROGRESS_REPORTER::WX_PROGRESS_REPORTER( wxWindow* aParent,
+        while( m_progress < m_maxProgress && m_maxProgress > 0 )
-        const wxString& aTitle, int aNumPhases ) :
+        {
+            if( !updateUI() )
+                return false;
+
+            wxMilliSleep( 20 );
+        }
+        return true;
+    }
+    else
+    {
+        wxMilliSleep( 20 );
+
+        return updateUI();
+    }
+}
+
+
+WX_PROGRESS_REPORTER::WX_PROGRESS_REPORTER( wxWindow* aParent, const wxString& aTitle,
+                                            int aNumPhases ) :
     PROGRESS_REPORTER( aNumPhases ),
-    wxProgressDialog( aTitle, wxT( "" ), 1, aParent, wxPD_AUTO_HIDE | wxPD_CAN_ABORT |
+    wxProgressDialog( aTitle, wxT( "" ), 1, aParent,
+                      // wxPD_APP_MODAL |   // Don't use; messes up OSX when called from
+                                            // quasi-modal dialog
+                      wxPD_AUTO_HIDE |      // *MUST* use; otherwise wxWidgets will spin
+                                            // up another event loop on completion which
+                                            // causes all sorts of grief
+                      wxPD_CAN_ABORT |
                       wxPD_ELAPSED_TIME )
 {
 }
@@ -99,30 +120,21 @@ WX_PROGRESS_REPORTER::~WX_PROGRESS_REPORTER()
 }
 
 
-void WX_PROGRESS_REPORTER::updateUI()
+bool WX_PROGRESS_REPORTER::updateUI()
 {
     int cur = currentProgress();
 
     if( cur < 0 || cur > 1000 )
         cur = 0;
 
+    wxString message;
+    {
+        std::lock_guard<std::mutex> guard( m_mutex );
+        message = m_rptMessage;
+    }
+
     SetRange( 1000 );
-    wxProgressDialog::Update( cur, m_rptMessage );
+    return wxProgressDialog::Update( cur, message );
 }
 
 
-void PROGRESS_REPORTER::KeepRefreshing( bool aWait )
-{
-    #ifdef USE_OPENMP
-        while( m_progress < m_maxProgress && m_maxProgress > 0 )
-        {
-            updateUI();
-            wxMilliSleep( 10 );
-
-            if( !aWait )
-                return;
-        }
-    #else
-        updateUI();
-    #endif
-}

cvpcb/cvpcb_mainframe.cpp

@@ -42,6 +42,7 @@
 #include <fp_lib_table.h>
 #include <netlist_reader.h>
 #include <bitmaps.h>
+#include <widgets/progress_reporter.h>
 
 #include <cvpcb_mainframe.h>
 #include <cvpcb.h>
@@ -713,11 +714,9 @@ bool CVPCB_MAINFRAME::LoadFootprintFiles()
         return false;
     }
 
-    {
+    WX_PROGRESS_REPORTER progressReporter( this, _( "Loading Footprint Libraries" ), 2 );
-    wxBusyCursor dummy;  // Let the user know something is happening.
 
-    m_FootprintsList->ReadFootprintFiles( fptbl );
+    m_FootprintsList->ReadFootprintFiles( fptbl, nullptr, &progressReporter );
-    }
 
     if( m_FootprintsList->GetErrorCount() )
     {

include/footprint_info.h

@@ -50,6 +50,7 @@ class FP_LIB_TABLE;
 class FOOTPRINT_LIST;
 class FOOTPRINT_LIST_IMPL;
 class FOOTPRINT_ASYNC_LOADER;
+class WX_PROGRESS_REPORTER;
 class wxTopLevelWindow;
 class KIWAY;
 
@@ -247,11 +248,14 @@ public:
      * @param aTable defines all the libraries.
      * @param aNickname is the library to read from, or if NULL means read all
      *         footprints from all known libraries in aTable.
+     * @param aProgressReporter is an optional progress reporter.  ReadFootprintFiles()
+     *         will use 2 phases within the reporter.
      * @return bool - true if it ran to completion, else false if it aborted after
      *  some number of errors.  If true, it does not mean there were no errors, check
      *  GetErrorCount() for that, should be zero to indicate success.
      */
-    virtual bool ReadFootprintFiles( FP_LIB_TABLE* aTable, const wxString* aNickname = NULL ) = 0;
+    virtual bool ReadFootprintFiles( FP_LIB_TABLE* aTable, const wxString* aNickname = nullptr,
+                                     WX_PROGRESS_REPORTER* aProgressReporter = nullptr ) = 0;
 
     void DisplayErrors( wxTopLevelWindow* aCaller = NULL );
 

include/widgets/progress_reporter.h

@@ -30,6 +30,12 @@
 
 #include <wx/progdlg.h>
 
+/**
+ * A progress reporter for use in multi-threaded environments.  The various advancement
+ * and message methods can be called from sub-threads.  The KeepRefreshing method *MUST*
+ * be called only from the main thread (primarily a MSW requirement, which won't allow
+ * access to UI objects allocated from a separate thread).
+ */
 class PROGRESS_REPORTER
 {
     public:
@@ -64,28 +70,26 @@ class PROGRESS_REPORTER
         void AdvanceProgress();
 
         /**
-         * Update the UI dialog.
+         * Update the UI dialog.  *MUST* only be called from the main thread.
-         * This function is compatible with OPENMP use.
+         * Returns false if the user clicked Cancel.
          */
-        void KeepRefreshing( bool aWait = false );
+        bool KeepRefreshing( bool aWait = false );
 
     protected:
 
         int currentProgress() const;
-        virtual void updateUI() = 0;
 
-        wxString m_rptMessage;
+        virtual bool updateUI() = 0;
-        int m_phase;
+
-        int m_numPhases;
+        wxString           m_rptMessage;
-        std::atomic_int m_progress;
+        mutable std::mutex m_mutex;
-        int m_maxProgress;
+        std::atomic_int    m_phase;
+        std::atomic_int    m_numPhases;
+        std::atomic_int    m_progress;
+        std::atomic_int    m_maxProgress;
 };
 
-/**
+
- * This class implements a wxProgressDialog that can be used in a OPENMP environment
- * (i.e. the progress bar update can be called from different theads).
- * It is mainly used in Zone fill calculations
- */
 class WX_PROGRESS_REPORTER : public PROGRESS_REPORTER, public wxProgressDialog
 {
 public:
@@ -104,7 +108,7 @@ public:
 
 private:
 
-    virtual void updateUI() override;
+    virtual bool updateUI() override;
 };
 
 #endif

pcbnew/footprint_info_impl.cpp

@@ -36,6 +36,7 @@
 #include <make_unique.h>
 #include <pgm_base.h>
 #include <wildcards_and_files_ext.h>
+#include <widgets/progress_reporter.h>
 
 #include <thread>
 
@@ -100,7 +101,7 @@ void FOOTPRINT_LIST_IMPL::loader_job()
 {
     wxString nickname;
 
-    while( m_queue_in.pop( nickname ) )
+    while( m_queue_in.pop( nickname ) && !m_cancelled )
     {
         CatchErrors( [this, &nickname]() {
             m_lib_table->PrefetchLib( nickname );
@@ -108,33 +109,68 @@ void FOOTPRINT_LIST_IMPL::loader_job()
         } );
 
         m_count_finished.fetch_add( 1 );
-    }
 
-    if( !m_first_to_finish.exchange( true ) )
+        if( m_progress_reporter )
-    {
+            m_progress_reporter->AdvanceProgress();
-        // yay, we're first to finish!
-        if( m_loader->m_completion_cb )
-        {
-            m_loader->m_completion_cb();
-        }
     }
 }
 
 
-bool FOOTPRINT_LIST_IMPL::ReadFootprintFiles( FP_LIB_TABLE* aTable, const wxString* aNickname )
+bool FOOTPRINT_LIST_IMPL::ReadFootprintFiles( FP_LIB_TABLE* aTable, const wxString* aNickname,
+                                              WX_PROGRESS_REPORTER* aProgressReporter )
 {
     if( aTable->GenLastModifiedChecksum( aNickname ) == m_libraries_last_mod_checksum )
         return true;
 
+    m_progress_reporter = aProgressReporter;
+    m_cancelled = false;
+
     FOOTPRINT_ASYNC_LOADER loader;
 
     loader.SetList( this );
     loader.Start( aTable, aNickname );
-    bool retval = loader.Join();
+
+    if( m_progress_reporter )
+    {
+        m_progress_reporter->SetMaxProgress( m_queue_in.size() );
+        m_progress_reporter->Report( _( "Fetching Footprint Libraries" ) );
+    }
+
+    while( !m_cancelled && loader.GetProgress() < 100 )
+    {
+        if( m_progress_reporter )
+            m_cancelled = !m_progress_reporter->KeepRefreshing();
+        else
+            wxMilliSleep( 20 );
+    }
+
+    if( m_progress_reporter )
+        m_progress_reporter->AdvancePhase();
+
+    if( !m_cancelled )
+    {
+        if( m_progress_reporter )
+        {
+            m_progress_reporter->SetMaxProgress( m_queue_out.size() );
+            m_progress_reporter->Report( _( "Loading Footprints" ) );
+        }
+
+        loader.Join();
+    }
+
+    if( m_progress_reporter )
+        m_progress_reporter->AdvancePhase();
+
+    if( m_cancelled )
+    {
+        m_errors.move_push( std::make_unique<IO_ERROR>
+                    ( _( "Loading incomplete; cancelled by user." ), nullptr, nullptr, 0 ) );
+    }
 
     m_libraries_last_mod_checksum = aTable->GenLastModifiedChecksum( aNickname );
+    m_progress_reporter = nullptr;
 
-    return retval;
+    return m_errors.empty();
 }
 
 
@@ -145,7 +181,6 @@ void FOOTPRINT_LIST_IMPL::StartWorkers( FP_LIB_TABLE* aTable, wxString const* aN
     m_lib_table = aTable;
 
     // Clear data before reading files
-    m_first_to_finish.store( false );
     m_count_finished.store( 0 );
     m_errors.clear();
     m_list.clear();
@@ -176,6 +211,9 @@ bool FOOTPRINT_LIST_IMPL::JoinWorkers()
 
     m_threads.clear();
     m_queue_in.clear();
+    m_count_finished.store( 0 );
+
+    size_t total_count = m_queue_out.size();
 
     LOCALE_IO toggle_locale;
 
@@ -194,7 +232,7 @@ bool FOOTPRINT_LIST_IMPL::JoinWorkers()
         threads.push_back( std::thread( [this, &queue_parsed]() {
             wxString nickname;
 
-            while( this->m_queue_out.pop( nickname ) )
+            while( this->m_queue_out.pop( nickname ) && !m_cancelled )
             {
                 wxArrayString fpnames;
 
@@ -220,15 +258,29 @@ bool FOOTPRINT_LIST_IMPL::JoinWorkers()
                     }
                 }
 
-                for( auto const& fpname : fpnames )
+                for( int i = 0; i < fpnames.size() && !m_cancelled; i++ )
                 {
+                    wxString fpname = fpnames[ i ];
                     FOOTPRINT_INFO* fpinfo = new FOOTPRINT_INFO_IMPL( this, nickname, fpname );
                     queue_parsed.move_push( std::unique_ptr<FOOTPRINT_INFO>( fpinfo ) );
                 }
+
+                if( m_progress_reporter )
+                    m_progress_reporter->AdvanceProgress();
+
+                m_count_finished.fetch_add( 1 );
             }
         } ) );
     }
 
+    while( !m_cancelled && m_count_finished.load() < total_count )
+    {
+        if( m_progress_reporter )
+            m_cancelled = !m_progress_reporter->KeepRefreshing();
+        else
+            wxMilliSleep( 20 );
+    }
+
     for( auto& thr : threads )
         thr.join();
 
@@ -251,7 +303,10 @@ size_t FOOTPRINT_LIST_IMPL::CountFinished()
 }
 
 
-FOOTPRINT_LIST_IMPL::FOOTPRINT_LIST_IMPL() : m_loader( nullptr ), m_first_to_finish( false ), m_count_finished( 0 )
+FOOTPRINT_LIST_IMPL::FOOTPRINT_LIST_IMPL() :
+    m_loader( nullptr ),
+    m_count_finished( 0 ),
+    m_progress_reporter( nullptr )
 {
 }
 

pcbnew/footprint_info_impl.h

@@ -28,6 +28,7 @@
 
 #include <footprint_info.h>
 #include <sync_queue.h>
+#include <widgets/progress_reporter.h>
 
 class LOCALE_IO;
 
@@ -60,9 +61,10 @@ class FOOTPRINT_LIST_IMPL : public FOOTPRINT_LIST
     std::vector<std::thread> m_threads;
     SYNC_QUEUE<wxString>     m_queue_in;
     SYNC_QUEUE<wxString>     m_queue_out;
-    std::atomic_bool         m_first_to_finish;
     std::atomic_size_t       m_count_finished;
     long long                m_libraries_last_mod_checksum;
+    WX_PROGRESS_REPORTER*    m_progress_reporter;
+    std::atomic_bool         m_cancelled;
 
     /**
      * Call aFunc, pushing any IO_ERRORs and std::exceptions it throws onto m_errors.
@@ -87,8 +89,8 @@ public:
     FOOTPRINT_LIST_IMPL();
     virtual ~FOOTPRINT_LIST_IMPL();
 
-    virtual bool ReadFootprintFiles(
+    virtual bool ReadFootprintFiles( FP_LIB_TABLE* aTable, const wxString* aNickname = nullptr,
-            FP_LIB_TABLE* aTable, const wxString* aNickname = NULL ) override;
+                                     WX_PROGRESS_REPORTER* aProgressReporter = nullptr ) override;
 };
 
 #endif // FOOTPRINT_INFO_IMPL_H

pcbnew/zone_filler.cpp

@@ -114,23 +114,9 @@ void ZONE_FILLER::Fill( std::vector<ZONE_CONTAINER*> aZones )
         m_progressReporter->SetMaxProgress( toFill.size() );
     }
 
-
+    m_count_done = 0;
-    #ifdef USE_OPENMP
+    std::thread fillWorker( [ this, toFill ]()
-        // launch at least two threads, one to compute, second to update UI
-        #pragma omp parallel num_threads( std::max( omp_get_num_procs(), 2 ) )
-    #endif
     {
-        #ifdef USE_OPENMP
-            #pragma omp master
-            if( m_progressReporter )
-            {
-                m_progressReporter->KeepRefreshing( true );
-            }
-        #endif
-
-        #ifdef USE_OPENMP
-            #pragma omp for schedule(dynamic)
-        #endif
         for( unsigned i = 0; i < toFill.size(); i++ )
         {
             SHAPE_POLY_SET rawPolys, finalPolys;
@@ -142,17 +128,28 @@ void ZONE_FILLER::Fill( std::vector<ZONE_CONTAINER*> aZones )
             toFill[i].m_zone->SetIsFilled( true );
 
             if( m_progressReporter )
-            {
                 m_progressReporter->AdvanceProgress();
-            }
+
+            m_count_done.fetch_add( 1 );
         }
+    } );
+
+    while( m_count_done.load() < toFill.size() )
+    {
+        if( m_progressReporter )
+            m_progressReporter->KeepRefreshing();
+        else
+            wxMilliSleep( 20 );
     }
 
+    fillWorker.join();
+
     // Now remove insulated copper islands
     if( m_progressReporter )
     {
         m_progressReporter->AdvancePhase();
         m_progressReporter->Report( _( "Removing insulated copper islands..." ) );
+        m_progressReporter->KeepRefreshing();
     }
 
     connectivity->SetProgressReporter( m_progressReporter );
@@ -177,33 +174,31 @@ void ZONE_FILLER::Fill( std::vector<ZONE_CONTAINER*> aZones )
         m_progressReporter->Report( _( "Caching polygon triangulations..." ) );
         m_progressReporter->SetMaxProgress( toFill.size() );
     }
-    #ifdef USE_OPENMP
-        // launch at least two threads, one to compute, second to update UI
-        #pragma omp parallel num_threads( std::max( omp_get_num_procs(), 2 ) )
-    #endif
-    {
-        #ifdef USE_OPENMP
-            #pragma omp master
-            if( m_progressReporter )
-            {
-                m_progressReporter->KeepRefreshing( true );
-            }
-        #endif
 
-        #ifdef USE_OPENMP
+    m_count_done = 0;
-            #pragma omp for schedule(dynamic)
+    std::thread triangulationWorker( [ this, toFill ]()
-        #endif
+    {
         for( unsigned i = 0; i < toFill.size(); i++ )
         {
             if( m_progressReporter )
-            {
                 m_progressReporter->AdvanceProgress();
-            }
 
             toFill[i].m_zone->CacheTriangulation();
+
+            m_count_done.fetch_add( 1 );
         }
+    } );
+
+    while( m_count_done.load() < toFill.size() )
+    {
+        if( m_progressReporter )
+            m_progressReporter->KeepRefreshing();
+        else
+            wxMilliSleep( 10 );
     }
 
+    triangulationWorker.join();
+
     // If some zones must be filled by segments, create the filling segments
     // (note, this is a outdated option, but it exists)
     int zones_to_fill_count = 0;

pcbnew/zone_filler.h

@@ -119,6 +119,7 @@ private:
     BOARD* m_board;
     COMMIT* m_commit;
     PROGRESS_REPORTER* m_progressReporter;
+    std::atomic_size_t m_count_done;
 };
 
 #endif

pcbnew/zones_by_polygon_fill_functions.cpp

@@ -103,17 +103,11 @@ int PCB_EDIT_FRAME::Fill_All_Zones( wxWindow * aActiveWindow )
 
     ZONE_FILLER filler( GetBoard() );
 
-    // progressReporter must be created *only* if needed
+    std::unique_ptr<WX_PROGRESS_REPORTER> progressReporter(
-    if( aActiveWindow )
+        new WX_PROGRESS_REPORTER( aActiveWindow, _( "Fill All Zones" ), 3 ) );
-    {
-        std::unique_ptr<WX_PROGRESS_REPORTER> progressReporter(
-            new WX_PROGRESS_REPORTER( aActiveWindow, _( "Fill All Zones" ), 3 ) );
 
-        filler.SetProgressReporter( progressReporter.get() );
+    filler.SetProgressReporter( progressReporter.get() );
-        filler.Fill( toFill );
+    filler.Fill( toFill );
-    }
-    else    // do not use a WX_PROGRESS_REPORTER in ZONE_FILLER instance
-        filler.Fill( toFill );
 
     return 0;
 }