From f68bf33cd3b5cb8d104afacdb03a4349ad1680d8 Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Sat, 10 Feb 2018 13:32:57 +0000 Subject: [PATCH] Implement progress reporter for cvpcb footprint loading. Fixes: lp:1676910 * https://bugs.launchpad.net/kicad/+bug/1676910 --- common/widgets/progress_reporter.cpp | 80 +++++++++++--------- cvpcb/cvpcb_mainframe.cpp | 7 +- include/footprint_info.h | 6 +- include/widgets/progress_reporter.h | 34 +++++---- pcbnew/footprint_info_impl.cpp | 87 ++++++++++++++++++---- pcbnew/footprint_info_impl.h | 8 +- pcbnew/zone_filler.cpp | 65 ++++++++-------- pcbnew/zone_filler.h | 1 + pcbnew/zones_by_polygon_fill_functions.cpp | 14 +--- 9 files changed, 184 insertions(+), 118 deletions(-) diff --git a/common/widgets/progress_reporter.cpp b/common/widgets/progress_reporter.cpp index 53068bf7c8..ddd46668b8 100644 --- a/common/widgets/progress_reporter.cpp +++ b/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_progress = 0; - updateUI(); + m_phase.store( aPhase ); + m_progress.store( 0 ); } void PROGRESS_REPORTER::AdvancePhase( ) { - m_phase++; - m_progress = 0; - updateUI(); + m_phase.fetch_add( 1 ); + m_progress.store( 0 ); } void PROGRESS_REPORTER::Report( const wxString& aMessage ) { + std::lock_guard guard( m_mutex ); m_rptMessage = aMessage; - updateUI(); } void PROGRESS_REPORTER::SetMaxProgress( int aMaxProgress ) { - m_maxProgress = aMaxProgress; - updateUI(); + m_maxProgress.store( aMaxProgress ); } 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 -// (without this option the PROGRESS_REPORTER is modal for the parent frame) -// and PROGRESS_REPORTER works fine on OSX only without this style -// when called from a quasi modal dialog -WX_PROGRESS_REPORTER::WX_PROGRESS_REPORTER( wxWindow* aParent, - const wxString& aTitle, int aNumPhases ) : +bool PROGRESS_REPORTER::KeepRefreshing( bool aWait ) +{ + if( aWait ) + { + while( m_progress < m_maxProgress && m_maxProgress > 0 ) + { + 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 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 -} diff --git a/cvpcb/cvpcb_mainframe.cpp b/cvpcb/cvpcb_mainframe.cpp index 754270b067..b6a64ee2c1 100644 --- a/cvpcb/cvpcb_mainframe.cpp +++ b/cvpcb/cvpcb_mainframe.cpp @@ -42,6 +42,7 @@ #include #include #include +#include #include #include @@ -713,11 +714,9 @@ bool CVPCB_MAINFRAME::LoadFootprintFiles() return false; } - { - wxBusyCursor dummy; // Let the user know something is happening. + WX_PROGRESS_REPORTER progressReporter( this, _( "Loading Footprint Libraries" ), 2 ); - m_FootprintsList->ReadFootprintFiles( fptbl ); - } + m_FootprintsList->ReadFootprintFiles( fptbl, nullptr, &progressReporter ); if( m_FootprintsList->GetErrorCount() ) { diff --git a/include/footprint_info.h b/include/footprint_info.h index 94b618f8ea..469199971e 100644 --- a/include/footprint_info.h +++ b/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 ); diff --git a/include/widgets/progress_reporter.h b/include/widgets/progress_reporter.h index 5504b5c792..2eff0f217b 100644 --- a/include/widgets/progress_reporter.h +++ b/include/widgets/progress_reporter.h @@ -30,6 +30,12 @@ #include +/** + * 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. - * This function is compatible with OPENMP use. + * Update the UI dialog. *MUST* only be called from the main thread. + * 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; - int m_phase; - int m_numPhases; - std::atomic_int m_progress; - int m_maxProgress; + virtual bool updateUI() = 0; + + wxString m_rptMessage; + mutable std::mutex m_mutex; + 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 diff --git a/pcbnew/footprint_info_impl.cpp b/pcbnew/footprint_info_impl.cpp index 4afb7159fd..a5ba83c166 100644 --- a/pcbnew/footprint_info_impl.cpp +++ b/pcbnew/footprint_info_impl.cpp @@ -36,6 +36,7 @@ #include #include #include +#include #include @@ -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 ) ) - { - // yay, we're first to finish! - if( m_loader->m_completion_cb ) - { - m_loader->m_completion_cb(); - } + if( m_progress_reporter ) + m_progress_reporter->AdvanceProgress(); } } -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 + ( _( "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( 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 ) { } diff --git a/pcbnew/footprint_info_impl.h b/pcbnew/footprint_info_impl.h index 14c80acbac..fcaa76ff75 100644 --- a/pcbnew/footprint_info_impl.h +++ b/pcbnew/footprint_info_impl.h @@ -28,6 +28,7 @@ #include #include +#include class LOCALE_IO; @@ -60,9 +61,10 @@ class FOOTPRINT_LIST_IMPL : public FOOTPRINT_LIST std::vector m_threads; SYNC_QUEUE m_queue_in; SYNC_QUEUE 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( - FP_LIB_TABLE* aTable, const wxString* aNickname = NULL ) override; + virtual bool ReadFootprintFiles( FP_LIB_TABLE* aTable, const wxString* aNickname = nullptr, + WX_PROGRESS_REPORTER* aProgressReporter = nullptr ) override; }; #endif // FOOTPRINT_INFO_IMPL_H diff --git a/pcbnew/zone_filler.cpp b/pcbnew/zone_filler.cpp index 7797e4a81d..9bd5542022 100644 --- a/pcbnew/zone_filler.cpp +++ b/pcbnew/zone_filler.cpp @@ -114,23 +114,9 @@ void ZONE_FILLER::Fill( std::vector aZones ) 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 + m_count_done = 0; + std::thread fillWorker( [ this, toFill ]() { - #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 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 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 - #pragma omp for schedule(dynamic) - #endif + m_count_done = 0; + std::thread triangulationWorker( [ this, toFill ]() + { 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; diff --git a/pcbnew/zone_filler.h b/pcbnew/zone_filler.h index 82a3731871..2af20a5b0c 100644 --- a/pcbnew/zone_filler.h +++ b/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 diff --git a/pcbnew/zones_by_polygon_fill_functions.cpp b/pcbnew/zones_by_polygon_fill_functions.cpp index d769c469fd..bfba818af5 100644 --- a/pcbnew/zones_by_polygon_fill_functions.cpp +++ b/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 - if( aActiveWindow ) - { - std::unique_ptr progressReporter( - new WX_PROGRESS_REPORTER( aActiveWindow, _( "Fill All Zones" ), 3 ) ); + std::unique_ptr progressReporter( + new WX_PROGRESS_REPORTER( aActiveWindow, _( "Fill All Zones" ), 3 ) ); - filler.SetProgressReporter( progressReporter.get() ); - filler.Fill( toFill ); - } - else // do not use a WX_PROGRESS_REPORTER in ZONE_FILLER instance - filler.Fill( toFill ); + filler.SetProgressReporter( progressReporter.get() ); + filler.Fill( toFill ); return 0; }