Move gui locking to GUIHandler

client/CPlayerInterface.cpp

@@ -78,7 +78,6 @@ void processCommand(const std::string &message, CClient *&client);
 extern std::queue<SDL_Event> events;
 extern boost::mutex eventsM;
 boost::recursive_mutex * CPlayerInterface::pim = new boost::recursive_mutex;
-CondSh<bool> CPlayerInterface::terminate_cond;
 
 CPlayerInterface * LOCPLINT;
 
@@ -112,14 +111,13 @@ CPlayerInterface::CPlayerInterface(PlayerColor Player)
 	makingTurn = false;
 	showingDialog = new CondSh<bool>(false);
 	cingconsole = new CInGameConsole;
-	terminate_cond.set(false);
+	GH.terminate_cond.set(false);
 	firstCall = 1; //if loading will be overwritten in serialize
 	autosaveCount = 0;
 	isAutoFightOn = false;
 
 	duringMovement = false;
-	ignoreEvents = false;
+	ignoreEvents = false;	
-	locked = false;
 }
 
 CPlayerInterface::~CPlayerInterface()
@@ -1590,11 +1588,13 @@ void CPlayerInterface::setSelection(const CArmedInstance * obj)
 
 void CPlayerInterface::update()
 {
-	if (!locked)
+	// Make sure that gamestate won't change when GUI objects may obtain its parts on event processing or drawing request
-	{
+	boost::shared_lock<boost::shared_mutex> gsLock(cb->getGsMutex());
-		logGlobal->errorStream() << "Non synchronized update of PlayerInterface";
+	
+	// While mutexes were locked away we may be have stopped being the active interface	
+	if(LOCPLINT != this)
 		return;
-	}
+	
 	//if there are any waiting dialogs, show them
 	if((howManyPeople <= 1 || makingTurn) && !dialogs.empty() && !showingDialog->get())
 	{
@@ -1622,38 +1622,6 @@ void CPlayerInterface::update()
 		GH.drawFPSCounter();
 }
 
-void CPlayerInterface::runLocked(std::function<void()> functor)
-{
-	// Updating GUI requires locking pim mutex (that protects screen and GUI state).
-	// When ending the game, the pim mutex might be hold by other thread,
-	// that will notify us about the ending game by setting terminate_cond flag.
-
-	bool acquiredTheLockOnPim = false; //for tracking whether pim mutex locking succeeded
-	while(!terminate_cond.get() && !(acquiredTheLockOnPim = pim->try_lock())) //try acquiring long until it succeeds or we are told to terminate
-		boost::this_thread::sleep(boost::posix_time::milliseconds(15));
-
-	if(!acquiredTheLockOnPim)
-	{
-		// We broke the while loop above and not because of mutex, so we must be terminating.
-		assert(terminate_cond.get());
-		return;
-	}
-
-	// If we are here, pim mutex has been successfully locked - let's store it in a safe RAII lock.
-	boost::unique_lock<boost::recursive_mutex> un(*pim, boost::adopt_lock);
-
-	// While mutexes were locked away we may be have stopped being the active interface
-	if(LOCPLINT != this)
-		return;
-
-	// Make sure that gamestate won't change when GUI objects may obtain its parts on event processing or drawing request
-	boost::shared_lock<boost::shared_mutex> gsLock(cb->getGsMutex());
-
-	locked = true;
-	functor();
-	locked = false;
-}
-
 int CPlayerInterface::getLastIndex( std::string namePrefix)
 {
 	using namespace boost::filesystem;
@@ -2133,7 +2101,7 @@ void CPlayerInterface::gameOver(PlayerColor player, const EVictoryLossCheckResul
 		{
 			if(adventureInt)
 			{
-				terminate_cond.setn(true);
+				GH.terminate_cond.setn(true);
 				adventureInt->deactivate();
 				if(GH.topInt() == adventureInt)
 					GH.popInt(adventureInt);

client/CPlayerInterface.h

@@ -1,7 +1,6 @@
 #pragma once
 
 
-//#include "../lib/CondSh.h"
 #include "../lib/FunctionList.h"
 #include "../lib/CGameInterface.h"
 #include "../lib/NetPacksBase.h"
@@ -85,7 +84,7 @@ enum
 };
 
 /// Central class for managing user interface logic
-class CPlayerInterface : public CGameInterface, public ILockedUpdatable
+class CPlayerInterface : public CGameInterface, public IUpdateable
 {
 	const CArmedInstance * currentSelection;
 public:
@@ -136,7 +135,6 @@ public:
 	} spellbookSettings;
 
 	void update() override;
-	void runLocked(std::function<void()> functor) override;
 	void initializeHeroTownList();
 	int getLastIndex(std::string namePrefix);
 
@@ -271,10 +269,6 @@ public:
 	CPlayerInterface(PlayerColor Player);//c-tor
 	~CPlayerInterface();//d-tor
 
-	static CondSh<bool> terminate_cond; // confirm termination
-
-
-
 private:
 
 	template <typename Handler> void serializeTempl(Handler &h, const int version);
@@ -300,8 +294,6 @@ private:
 	bool duringMovement;
 	bool ignoreEvents;
 
-	bool locked;
-
 	void doMoveHero(const CGHeroInstance *h, CGPath path);
 };
 

client/CPreGame.cpp

@@ -516,7 +516,6 @@ void CGPreGame::disposeGraphics()
 
 void CGPreGame::update()
 {
-	boost::unique_lock<boost::recursive_mutex> lock(*CPlayerInterface::pim);
 	if(CGP != this) //don't update if you are not a main interface
 		return;
 
@@ -543,12 +542,6 @@ void CGPreGame::update()
 		GH.drawFPSCounter();
 }
 
-void CGPreGame::runLocked(std::function<void()> cb)
-{
-	boost::unique_lock<boost::recursive_mutex> lock(*CPlayerInterface::pim);
-	cb();	
-}
-
 void CGPreGame::openCampaignScreen(std::string name)
 {
 	if (vstd::contains(CGPreGameConfig::get().getCampaigns().Struct(), name))

client/CPreGame.h

@@ -597,7 +597,7 @@ private:
 };
 
 /// Handles background screen, loads graphics for victory/loss condition and random town or hero selection
-class CGPreGame : public CIntObject, public ILockedUpdatable
+class CGPreGame : public CIntObject, public IUpdateable
 {
 	void loadGraphics();
 	void disposeGraphics();
@@ -611,7 +611,6 @@ public:
 
 	~CGPreGame();
 	void update() override;
-	void runLocked(std::function<void()> cb) override;
 	void openSel(CMenuScreen::EState type, CMenuScreen::EMultiMode multi = CMenuScreen::SINGLE_PLAYER);
 
 	void openCampaignScreen(std::string name);

client/gui/CGuiHandler.cpp

@@ -1,5 +1,6 @@
 #include "StdInc.h"
 #include "CGuiHandler.h"
+#include "../lib/CondSh.h"
 
 #include <SDL.h>
 
@@ -15,6 +16,7 @@
 extern std::queue<SDL_Event> events;
 extern boost::mutex eventsM;
 
+CondSh<bool> CGuiHandler::terminate_cond;
 boost::thread_specific_ptr<bool> inGuiThread;
 
 SObjectConstruction::SObjectConstruction( CIntObject *obj )
@@ -389,25 +391,39 @@ void CGuiHandler::fakeMouseMove()
 
 void CGuiHandler::renderFrame()
 {
-	auto doUpdate = [this]()
 	{
-		if(nullptr != curInt)
+	// Updating GUI requires locking pim mutex (that protects screen and GUI state).
+	// During game:
+	// When ending the game, the pim mutex might be hold by other thread,
+	// that will notify us about the ending game by setting terminate_cond flag.		
+	//in PreGame terminate_cond stay false 
+		
+		bool acquiredTheLockOnPim = false; //for tracking whether pim mutex locking succeeded
+		while(!terminate_cond.get() && !(acquiredTheLockOnPim = CPlayerInterface::pim->try_lock())) //try acquiring long until it succeeds or we are told to terminate
+			boost::this_thread::sleep(boost::posix_time::milliseconds(15));
+
+		if(!acquiredTheLockOnPim)
 		{
-			curInt -> update();
+			// We broke the while loop above and not because of mutex, so we must be terminating.
-		}			
+			assert(terminate_cond.get());
+			return;
+		}
+
+		// If we are here, pim mutex has been successfully locked - let's store it in a safe RAII lock.
+		boost::unique_lock<boost::recursive_mutex> un(*CPlayerInterface::pim, boost::adopt_lock);
+
+		if(nullptr != curInt)
+			curInt->update();
+			
 		// draw the mouse cursor and update the screen
 		CCS->curh->render();
 
 		if(0 != SDL_RenderCopy(mainRenderer, screenTexture, nullptr, nullptr))
 			logGlobal->errorStream() << __FUNCTION__ << " SDL_RenderCopy " << SDL_GetError();
 
-		SDL_RenderPresent(mainRenderer);				
+		SDL_RenderPresent(mainRenderer);					
-	};
+	}
-	
+
-	if(curInt)
-		curInt->runLocked(doUpdate);
-	else
-		doUpdate();
 	
 	mainFPSmng->framerateDelay(); // holds a constant FPS	
 }
@@ -423,6 +439,8 @@ CGuiHandler::CGuiHandler()
 	// Creates the FPS manager and sets the framerate to 48 which is doubled the value of the original Heroes 3 FPS rate
 	mainFPSmng = new CFramerateManager(48);
 	//do not init CFramerateManager here --AVS
+	
+	terminate_cond.set(false);
 }
 
 CGuiHandler::~CGuiHandler()

client/gui/CGuiHandler.h

@@ -8,9 +8,9 @@ class CFramerateManager;
 class CGStatusBar;
 class CIntObject;
 class IUpdateable;
-class ILockedUpdatable;
 class IShowActivatable;
 class IShowable;
+template <typename T> struct CondSh;
 
 /*
  * CGuiHandler.h, part of VCMI engine
@@ -72,7 +72,7 @@ public:
 	std::vector<IShowable*> objsToBlit;
 
 	SDL_Event * current; //current event - can be set to nullptr to stop handling event
-	ILockedUpdatable *curInt;
+	IUpdateable *curInt;
 
 	Point lastClick;
 	unsigned lastClickTime;
@@ -109,6 +109,8 @@ public:
 	static bool isArrowKey(SDL_Keycode key);
 	static bool amIGuiThread();
 	static void pushSDLEvent(int type, int usercode = 0);
+	
+	static CondSh<bool> terminate_cond; // confirm termination
 };
 
 extern CGuiHandler GH; //global gui handler

client/gui/CIntObject.h

@@ -38,13 +38,6 @@ public:
 	virtual ~IUpdateable(){}; //d-tor
 };
 
-class ILockedUpdatable: public IUpdateable
-{
-public:
-	virtual void runLocked(std::function<void()> cb) = 0;
-	virtual ~ILockedUpdatable(){}; //d-tor
-};
-
 // Defines a show method
 class IShowable
 {