Loading Screen: refactor and clean up implementation

This covers several things:

Instead of creating a new worker thread manually and having it set class member state flags,
I instead utilized std::future and std::async. As far as I can tell, this should avoid the
(albeit unlikely) potential race condition wherein the ls could close before loading at all
if the thread was delayed in starting.

Removed the timer interface and split it into two components:
* A drawing callback that handles the animation. This has its granularity retained at 100 ms,
  but can be easily adjusted if we want it to go faster.
* A pump monitor that runs much more frequently (every time events::run_event_loop is called,
  technically). This handles checking the state of the std::future object and will then close
  the window. This makes the loading screen a bit speedier compared to the previous iteration
  since it no longer wastes time waiting for the next timer callback to close.

The one bit I'm not 100% sure about is the check for the future object's ready state in the dtor.
Though, relatedly, I wonder if perhaps we should wait for the worker thread to complete in that
case instead of exiting....

Also shuffled a few things around, such as immediately setting enter/esc disabled in pre_show
instead of waiting for everything there to finish.

Do note there occasionally seem to be a few odd issues with the dot animation after this commit.
Will look into that. But it may be some texture cache-related issue...

This also cleans up some unused log defines and a VS 2013 conditional include.

src/gui/dialogs/loading_screen.cpp

@@ -32,17 +32,13 @@
 #include "utils/functional.hpp"
 #include "video.hpp"
 
+#include <chrono>
 #include <cstdlib>
-
+#include <limits>
-#if defined(_MSC_VER) && _MSC_VER <= 1800
-#include <Windows.h>
-#endif
 
 static lg::log_domain log_loadscreen("loadscreen");
 #define ERR_LS LOG_STREAM(err, log_loadscreen)
 #define WRN_LS LOG_STREAM(warn, log_loadscreen)
-#define LOG_LS LOG_STREAM(info, log_loadscreen)
-#define DBG_LS LOG_STREAM(debug, log_loadscreen)
 
 static const std::map<loading_stage, std::string> stage_names {
 	{ loading_stage::build_terrain,       N_("Building terrain rules") },
@@ -76,11 +72,13 @@ namespace dialogs
 {
 REGISTER_DIALOG(loading_screen)
 
+loading_screen* loading_screen::current_load = nullptr;
+
 loading_screen::loading_screen(std::function<void()> f)
-	: timer_id_(0)
+	: animation_counter_(0)
-	, animation_counter_(0)
+	, next_animation_time_(std::numeric_limits<uint32_t>::max())
-	, work_(f)
+	, load_func_(f)
-	, worker_()
+	, worker_result_()
 	, cursor_setter_()
 	, progress_stage_label_(nullptr)
 	, animation_label_(nullptr)
@@ -88,16 +86,17 @@ loading_screen::loading_screen(std::function<void()> f)
 	, visible_stages_()
 	, animation_stages_()
 	, current_visible_stage_()
-	, is_worker_running_(false)
 {
 	for(const auto& pair : stage_names) {
 		visible_stages_[pair.first] = t_string(pair.second, "wesnoth-lib") + "...";
 	}
 
+	animation_stages_.reserve(20);
+
 	for(int i = 0; i != 20; ++i) {
 		std::string s(20, ' ');
 		s[i] = '.';
-		animation_stages_.push_back(s);
+		animation_stages_.push_back(std::move(s));
 	}
 
 	current_visible_stage_ = visible_stages_.end();
@@ -106,66 +105,62 @@ loading_screen::loading_screen(std::function<void()> f)
 
 void loading_screen::pre_show(window& window)
 {
-	if(work_) {
+	window.set_enter_disabled(true);
-		worker_.reset(new std::thread([this]() {
+	window.set_escape_disabled(true);
-			is_worker_running_ = true;
 
-			try {
+	cursor_setter_.reset(new cursor::setter(cursor::WAIT));
-				work_();
-			} catch(...) {
-				// TODO: guard this with a mutex.
-				exception_ = std::current_exception();
-			}
 
-			is_worker_running_ = false;
+	if(load_func_) {
-		}));
+		// Run the load function in its own thread.
+		try {
+			worker_result_ = std::async(std::launch::async, [this]() {
+				try {
+					load_func_();
+				} catch(...) {
+					// TODO: guard this with a mutex.
+					exception_ = std::current_exception();
+				}
+			});
+		} catch(const std::system_error& e) {
+			ERR_LS << "Failed to create worker thread: " << e.what() << "\n";
+		}
 	}
 
-	timer_id_ = add_timer(100, std::bind(&loading_screen::timer_callback, this, std::ref(window)), true);
-	cursor_setter_.reset(new cursor::setter(cursor::WAIT));
 	progress_stage_label_ = find_widget<label>(&window, "status", false, true);
 	animation_label_ = find_widget<label>(&window, "test_animation", false, true);
 
-	window.set_enter_disabled(true);
+	// Add a draw callback to handle the animation, et al.
-	window.set_escape_disabled(true);
+	window.connect_signal<event::DRAW>(
+		std::bind(&loading_screen::draw_callback, this), event::dispatcher::front_child);
+
+	set_next_animation_time();
 }
 
 void loading_screen::post_show(window& /*window*/)
 {
-	worker_.reset();
-	clear_timer();
 	cursor_setter_.reset();
 }
 
 void loading_screen::progress(loading_stage stage)
 {
-	if(!current_load) {
+	if(current_load && stage != loading_stage::none) {
-		return;
-	}
-
-	if(stage != loading_stage::none) {
 		current_load->current_stage_.store(stage, std::memory_order_release);
 	}
 }
 
-loading_screen* loading_screen::current_load = nullptr;
+void loading_screen::process(events::pump_info&)
-
-void loading_screen::timer_callback(window& window)
 {
-	if(!work_ || !worker_ || !is_worker_running_) {
+	if(!load_func_ || loading_complete()) {
 		if(exception_) {
-			clear_timer();
 			std::rethrow_exception(exception_);
 		}
 
-		worker_->detach();
+		get_window()->close();
-		window.close();
-	}
-
-	if(!work_) {
-		return;
 	}
+}
 
+void loading_screen::draw_callback()
+{
 	loading_stage stage = current_stage_.load(std::memory_order_acquire);
 
 	if(stage != loading_stage::none && (current_visible_stage_ == visible_stages_.end() || stage != current_visible_stage_->first)) {
@@ -179,10 +174,16 @@ void loading_screen::timer_callback(window& window)
 		progress_stage_label_->set_label(iter->second);
 	}
 
+	if(SDL_GetTicks() < next_animation_time_) {
+		return;
+	}
+
 	++animation_counter_;
 	if(animation_counter_ % 2 == 0) {
 		animation_label_->set_label(animation_stages_[(animation_counter_ / 2) % animation_stages_.size()]);
 	}
+
+	set_next_animation_time();
 }
 
 loading_screen::~loading_screen()
@@ -197,7 +198,7 @@ loading_screen::~loading_screen()
 	 * to the worker functions (filesystem.cpp, config parsing code, etc.) and then use that
 	 * to end the thread faster.
 	 */
-	if(is_worker_running_) {
+	if(!loading_complete()) {
 #if defined(_LIBCPP_VERSION) || defined(__MINGW32__)
 		std::_Exit(0);
 #else
@@ -205,7 +206,6 @@ loading_screen::~loading_screen()
 #endif
 	}
 
-	clear_timer();
 	current_load = nullptr;
 }
 
@@ -223,12 +223,15 @@ void loading_screen::display(std::function<void()> f)
 	}
 }
 
-void loading_screen::clear_timer()
+bool loading_screen::loading_complete() const
 {
-	if(timer_id_ != 0) {
+	using namespace std::chrono_literals;
-		remove_timer(timer_id_);
+	return worker_result_.wait_for(0ms) == std::future_status::ready;
-		timer_id_ = 0;
+}
-	}
+
+void loading_screen::set_next_animation_time()
+{
+	next_animation_time_ = SDL_GetTicks() + 100;
 }
 
 } // namespace dialogs

src/gui/dialogs/loading_screen.hpp

@@ -14,12 +14,14 @@
 #pragma once
 
 #include "gui/dialogs/modal_dialog.hpp"
+
+#include "events.hpp"
 #include "tstring.hpp"
 
+#include <atomic>
+#include <future>
 #include <map>
 #include <vector>
-#include <atomic>
-#include <thread>
 
 namespace cursor
 {
@@ -61,13 +63,12 @@ enum class loading_stage
 
 namespace gui2
 {
-
 class label;
 class window;
 
 namespace dialogs
 {
-class loading_screen : public modal_dialog
+class loading_screen : public modal_dialog, public events::pump_monitor
 {
 public:
 	loading_screen(std::function<void()> f);
@@ -80,37 +81,47 @@ public:
 	static void progress(loading_stage stage = loading_stage::none);
 
 private:
-	std::size_t timer_id_;
+	/** Inherited from modal_dialog, implemented by REGISTER_DIALOG. */
-	int animation_counter_;
-	std::function<void()> work_;
-	std::unique_ptr<std::thread> worker_;
-	std::unique_ptr<cursor::setter> cursor_setter_;
-	std::exception_ptr exception_;
-	void clear_timer();
-
 	virtual const std::string& window_id() const override;
 
-	void timer_callback(window& window);
-
 	/** Inherited from modal_dialog. */
 	virtual void pre_show(window& window) override;
 
 	/** Inherited from modal_dialog. */
 	virtual void post_show(window& window) override;
 
+	/** Inherited from events::pump_monitor. */
+	virtual void process(events::pump_info&) override;
+
+	/** Checks whether the worker thread has returned and loading is complete. */
+	bool loading_complete() const;
+
+	/** Callback to handle drawing the progress animation. */
+	void draw_callback();
+
+	void set_next_animation_time();
+
+	int animation_counter_;
+	uint32_t next_animation_time_;
+
+	std::function<void()> load_func_;
+	std::future<void> worker_result_;
+	std::unique_ptr<cursor::setter> cursor_setter_;
+
+	std::exception_ptr exception_;
+
 	label* progress_stage_label_;
 	label* animation_label_;
+
 	static loading_screen* current_load;
 
 	std::atomic<loading_stage> current_stage_;
 
 	using stage_map = std::map<loading_stage, t_string>;
-
 	stage_map visible_stages_;
+
 	std::vector<t_string> animation_stages_;
 	stage_map::const_iterator current_visible_stage_;
-
-	bool is_worker_running_;
 };
 
 } // namespace dialogs