Replaced CPlayerInterface::pim with CGuiHandler::interfaceLock

- Removed CPlayerInterface::pim since this lock does not actually
protects LOCPLINT but rather entire game UI state
- added more logical CGuiHandler::interfaceLock
- interface lock is now non-recursive and is locked only once by initial
caller that want to access GUI

client/CMT.cpp

@@ -473,8 +473,6 @@ static void quitApplication()
 
 	vstd::clear_pointer(console);// should be removed after everything else since used by logging
 
-	boost::this_thread::sleep_for(boost::chrono::milliseconds(750));//???
-
 	if(!settings["session"]["headless"].Bool())
 		GH.screenHandler().close();
 
@@ -486,6 +484,10 @@ static void quitApplication()
 	}
 
 	std::cout << "Ending...\n";
+
+	// this method is always called from event/network threads, which keep interface mutex locked
+	// unlock it here to avoid assertion failure on GH descruction in exit()
+	GH.interfaceMutex.unlock();
 	exit(0);
 }
 

client/CPlayerInterface.cpp

@@ -113,8 +113,6 @@
 	if (isAutoFightOn && !battleInt)	\
 		return;
 
-boost::recursive_mutex * CPlayerInterface::pim = new boost::recursive_mutex;
-
 CPlayerInterface * LOCPLINT;
 
 std::shared_ptr<BattleInterface> CPlayerInterface::battleInt;
@@ -534,7 +532,6 @@ void CPlayerInterface::garrisonsChanged(ObjectInstanceID id1, ObjectInstanceID i
 
 void CPlayerInterface::garrisonsChanged(std::vector<const CGObjectInstance *> objs)
 {
-	boost::unique_lock<boost::recursive_mutex> un(*pim);
 	for (auto object : objs)
 	{
 		auto * hero = dynamic_cast<const CGHeroInstance*>(object);
@@ -754,7 +751,7 @@ void CPlayerInterface::activeStack(const BattleID & battleID, const CStack * sta
 		{
 			//FIXME: we want client rendering to proceed while AI is making actions
 			// so unlock mutex while AI is busy since this might take quite a while, especially if hero has many spells
-			auto unlockPim = vstd::makeUnlockGuard(*pim);
+			auto unlockPim = vstd::makeUnlockGuard(GH.interfaceMutex);
 			autofightingAI->activeStack(battleID, stack);
 			return;
 		}
@@ -770,11 +767,7 @@ void CPlayerInterface::activeStack(const BattleID & battleID, const CStack * sta
 		cb->battleMakeUnitAction(battleID, BattleAction::makeDefend(stack));
 	}
 
-	{
-		boost::unique_lock<boost::recursive_mutex> un(*pim);
-		battleInt->stackActivated(stack);
-		//Regeneration & mana drain go there
-	}
+	battleInt->stackActivated(stack);
 }
 
 void CPlayerInterface::battleEnd(const BattleID & battleID, const BattleResult *br, QueryID queryID)
@@ -1017,8 +1010,6 @@ void CPlayerInterface::showInfoDialogAndWait(std::vector<Component> & components
 
 void CPlayerInterface::showYesNoDialog(const std::string &text, CFunctionList<void()> onYes, CFunctionList<void()> onNo, const std::vector<std::shared_ptr<CComponent>> & components)
 {
-	boost::unique_lock<boost::recursive_mutex> un(*pim);
-
 	movementController->requestMovementAbort();
 	LOCPLINT->showingDialog->setn(true);
 	CInfoWindow::showYesNoDialog(text, components, onYes, onNo, playerID);
@@ -1116,7 +1107,6 @@ void CPlayerInterface::tileHidden(const std::unordered_set<int3> &pos)
 
 void CPlayerInterface::openHeroWindow(const CGHeroInstance *hero)
 {
-	boost::unique_lock<boost::recursive_mutex> un(*pim);
 	GH.windows().createAndPushWindow<CHeroWindow>(hero);
 }
 
@@ -1349,7 +1339,7 @@ void CPlayerInterface::waitWhileDialog(bool unlockPim)
 		return;
 	}
 
-	auto unlock = vstd::makeUnlockGuardIf(*pim, unlockPim);
+	auto unlock = vstd::makeUnlockGuardIf(GH.interfaceMutex, unlockPim);
 	boost::unique_lock<boost::mutex> un(showingDialog->mx);
 	while(showingDialog->data)
 		showingDialog->cond.wait(un);
@@ -1387,8 +1377,8 @@ void CPlayerInterface::centerView (int3 pos, int focusTime)
 	{
 		GH.windows().totalRedraw();
 		{
-			auto unlockPim = vstd::makeUnlockGuard(*pim);
 			IgnoreEvents ignore(*this);
+			auto unlockPim = vstd::makeUnlockGuard(GH.interfaceMutex);
 			boost::this_thread::sleep_for(boost::chrono::milliseconds(focusTime));
 		}
 	}
@@ -1825,7 +1815,7 @@ void CPlayerInterface::waitForAllDialogs(bool unlockPim)
 {
 	while(!dialogs.empty())
 	{
-		auto unlock = vstd::makeUnlockGuardIf(*pim, unlockPim);
+		auto unlock = vstd::makeUnlockGuardIf(GH.interfaceMutex, unlockPim);
 		boost::this_thread::sleep_for(boost::chrono::milliseconds(5));
 	}
 	waitWhileDialog(unlockPim);

client/CPlayerInterface.h

@@ -76,7 +76,6 @@ public: // TODO: make private
 	//minor interfaces
 	CondSh<bool> *showingDialog; //indicates if dialog box is displayed
 
-	static boost::recursive_mutex *pim;
 	bool makingTurn; //if player is already making his turn
 
 	CCastleInterface * castleInt; //nullptr if castle window isn't opened

client/CServerHandler.cpp

@@ -90,7 +90,7 @@ template<typename T> class CApplyOnLobby : public CBaseForLobbyApply
 public:
 	bool applyOnLobbyHandler(CServerHandler * handler, void * pack) const override
 	{
-		boost::unique_lock<boost::recursive_mutex> un(*CPlayerInterface::pim);
+		boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
 
 		T * ptr = static_cast<T *>(pack);
 		ApplyOnLobbyHandlerNetPackVisitor visitor(*handler);
@@ -296,7 +296,7 @@ void CServerHandler::applyPacksOnLobbyScreen()
 	boost::unique_lock<boost::recursive_mutex> lock(*mx);
 	while(!packsForLobbyScreen.empty())
 	{
-		boost::unique_lock<boost::recursive_mutex> guiLock(*CPlayerInterface::pim);
+		boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
 		CPackForLobby * pack = packsForLobbyScreen.front();
 		packsForLobbyScreen.pop_front();
 		CBaseForLobbyApply * apply = applier->getApplier(typeList.getTypeID(pack)); //find the applier

client/Client.cpp

@@ -367,7 +367,6 @@ void CClient::endGame()
 
 	GH.curInt = nullptr;
 	{
-		boost::unique_lock<boost::recursive_mutex> un(*CPlayerInterface::pim);
 		logNetwork->info("Ending current game!");
 		removeGUI();
 
@@ -499,8 +498,6 @@ std::string CClient::aiNameForPlayer(bool battleAI, bool alliedToHuman)
 
 void CClient::installNewPlayerInterface(std::shared_ptr<CGameInterface> gameInterface, PlayerColor color, bool battlecb)
 {
-	boost::unique_lock<boost::recursive_mutex> un(*CPlayerInterface::pim);
-
 	playerint[color] = gameInterface;
 
 	logGlobal->trace("\tInitializing the interface for player %s", color.toString());
@@ -513,8 +510,6 @@ void CClient::installNewPlayerInterface(std::shared_ptr<CGameInterface> gameInte
 
 void CClient::installNewBattleInterface(std::shared_ptr<CBattleGameInterface> battleInterface, PlayerColor color, bool needCallback)
 {
-	boost::unique_lock<boost::recursive_mutex> un(*CPlayerInterface::pim);
-
 	battleints[color] = battleInterface;
 
 	if(needCallback)
@@ -531,7 +526,7 @@ void CClient::handlePack(CPack * pack)
 	CBaseForCLApply * apply = applier->getApplier(typeList.getTypeID(pack)); //find the applier
 	if(apply)
 	{
-		boost::unique_lock<boost::recursive_mutex> guiLock(*CPlayerInterface::pim);
+		boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
 		apply->applyOnClBefore(this, pack);
 		logNetwork->trace("\tMade first apply on cl: %s", typeList.getTypeInfo(pack)->name());
 		gs->apply(pack);
@@ -614,7 +609,6 @@ void CClient::battleStarted(const BattleInfo * info)
 	{
 		if(att || def)
 		{
-			boost::unique_lock<boost::recursive_mutex> un(*CPlayerInterface::pim);
 			CPlayerInterface::battleInt = std::make_shared<BattleInterface>(info->getBattleID(), leftSide.armyObject, rightSide.armyObject, leftSide.hero, rightSide.hero, att, def);
 		}
 		else if(settings["session"]["spectate"].Bool() && !settings["session"]["spectate-skip-battle"].Bool())
@@ -622,7 +616,6 @@ void CClient::battleStarted(const BattleInfo * info)
 			//TODO: This certainly need improvement
 			auto spectratorInt = std::dynamic_pointer_cast<CPlayerInterface>(playerint[PlayerColor::SPECTATOR]);
 			spectratorInt->cb->onBattleStarted(info);
-			boost::unique_lock<boost::recursive_mutex> un(*CPlayerInterface::pim);
 			CPlayerInterface::battleInt = std::make_shared<BattleInterface>(info->getBattleID(), leftSide.armyObject, rightSide.armyObject, leftSide.hero, rightSide.hero, att, def, spectratorInt);
 		}
 	}
@@ -653,7 +646,7 @@ void CClient::startPlayerBattleAction(const BattleID & battleID, PlayerColor col
 	if(vstd::contains(battleints, color))
 	{
 		// we want to avoid locking gamestate and causing UI to freeze while AI is making turn
-		auto unlock = vstd::makeUnlockGuardIf(*CPlayerInterface::pim, !battleints[color]->human);
+		auto unlock = vstd::makeUnlockGuardIf(GH.interfaceMutex, !battleints[color]->human);
 
 		assert(vstd::contains(battleints, color));
 		battleints[color]->activeStack(battleID, gs->getBattle(battleID)->battleGetStackByID(gs->getBattle(battleID)->activeStack, false));

client/ClientCommandManager.cpp

@@ -74,7 +74,8 @@ void ClientCommandManager::handleGoSoloCommand()
 {
 	Settings session = settings.write["session"];
 
-	boost::unique_lock<boost::recursive_mutex> un(*CPlayerInterface::pim);
+	boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
+
 	if(!CSH->client)
 	{
 		printCommandMessage("Game is not in playing state");
@@ -120,7 +121,8 @@ void ClientCommandManager::handleControlaiCommand(std::istringstream& singleWord
 	singleWordBuffer >> colorName;
 	boost::to_lower(colorName);
 
-	boost::unique_lock<boost::recursive_mutex> un(*CPlayerInterface::pim);
+	boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
+
 	if(!CSH->client)
 	{
 		printCommandMessage("Game is not in playing state");
@@ -416,14 +418,6 @@ void ClientCommandManager::handleSetCommand(std::istringstream& singleWordBuffer
 	}
 }
 
-void ClientCommandManager::handleUnlockCommand(std::istringstream& singleWordBuffer)
-{
-	std::string mxname;
-	singleWordBuffer >> mxname;
-	if(mxname == "pim" && LOCPLINT)
-		LOCPLINT->pim->unlock();
-}
-
 void ClientCommandManager::handleCrashCommand()
 {
 	int* ptr = nullptr;
@@ -460,7 +454,7 @@ void ClientCommandManager::printCommandMessage(const std::string &commandMessage
 
 	if(currentCallFromIngameConsole)
 	{
-		boost::unique_lock<boost::recursive_mutex> un(*CPlayerInterface::pim);
+		boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
 		if(LOCPLINT && LOCPLINT->cingconsole)
 		{
 			LOCPLINT->cingconsole->print(commandMessage);
@@ -547,9 +541,6 @@ void ClientCommandManager::processCommand(const std::string & message, bool call
 	else if (commandName == "set")
 		handleSetCommand(singleWordBuffer);
 
-	else if(commandName == "unlock")
-		handleUnlockCommand(singleWordBuffer);
-
 	else if(commandName == "crash")
 		handleCrashCommand();
 

client/battle/BattleInterface.cpp

@@ -106,7 +106,6 @@ void BattleInterface::playIntroSoundAndUnlockInterface()
 {
 	auto onIntroPlayed = [this]()
 	{
-		boost::unique_lock<boost::recursive_mutex> un(*CPlayerInterface::pim);
 		if(LOCPLINT->battleInt)
 			onIntroSoundPlayed();
 	};
@@ -781,7 +780,7 @@ void BattleInterface::onAnimationsFinished()
 void BattleInterface::waitForAnimations()
 {
 	{
-		auto unlockPim = vstd::makeUnlockGuard(*CPlayerInterface::pim);
+		auto unlockPim = vstd::makeUnlockGuard(GH.interfaceMutex);
 		ongoingAnimationsState.waitUntil(false);
 	}
 

client/eventsSDL/InputHandler.cpp

@@ -113,32 +113,41 @@ bool InputHandler::ignoreEventsUntilInput()
 
 void InputHandler::preprocessEvent(const SDL_Event & ev)
 {
-	if((ev.type==SDL_QUIT) ||(ev.type == SDL_KEYDOWN && ev.key.keysym.sym==SDLK_F4 && (ev.key.keysym.mod & KMOD_ALT)))
+	if(ev.type == SDL_QUIT)
 	{
-#ifdef VCMI_ANDROID
+		boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
 		handleQuit(false);
-#else
-		handleQuit();
-#endif
 		return;
 	}
-#ifdef VCMI_ANDROID
-	else if (ev.type == SDL_KEYDOWN && ev.key.keysym.scancode == SDL_SCANCODE_AC_BACK)
+	else if(ev.type == SDL_KEYDOWN)
 	{
-		handleQuit(true);
-	}
-#endif
-	else if(ev.type == SDL_KEYDOWN && ev.key.keysym.sym==SDLK_F4)
-	{
-		boost::unique_lock<boost::recursive_mutex> lock(*CPlayerInterface::pim);
-		Settings full = settings.write["video"]["fullscreen"];
-		full->Bool() = !full->Bool();
+		if(ev.key.keysym.sym == SDLK_F4 && (ev.key.keysym.mod & KMOD_ALT))
+		{
+			boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
+			handleQuit(true);
+			return;
+		}
 
-		GH.onScreenResize();
-		return;
+		if(ev.key.keysym.scancode == SDL_SCANCODE_AC_BACK)
+		{
+			boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
+			handleQuit(true);
+			return;
+		}
+
+		if(ev.type == SDL_KEYDOWN && ev.key.keysym.sym == SDLK_F4)
+		{
+			boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
+			Settings full = settings.write["video"]["fullscreen"];
+			full->Bool() = !full->Bool();
+
+			GH.onScreenResize();
+			return;
+		}
 	}
 	else if(ev.type == SDL_USEREVENT)
 	{
+		boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
 		handleUserEvent(ev.user);
 
 		return;
@@ -149,21 +158,27 @@ void InputHandler::preprocessEvent(const SDL_Event & ev)
 		case SDL_WINDOWEVENT_RESTORED:
 #ifndef VCMI_IOS
 			{
-				boost::unique_lock<boost::recursive_mutex> lock(*CPlayerInterface::pim);
+				boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
 				GH.onScreenResize();
 			}
 #endif
 			break;
 		case SDL_WINDOWEVENT_FOCUS_GAINED:
-			if(settings["general"]["enableUiEnhancements"].Bool()) {
-				CCS->musich->setVolume(settings["general"]["music"].Integer());
-				CCS->soundh->setVolume(settings["general"]["sound"].Integer());
+			{
+				boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
+				if(settings["general"]["enableUiEnhancements"].Bool()) {
+					CCS->musich->setVolume(settings["general"]["music"].Integer());
+					CCS->soundh->setVolume(settings["general"]["sound"].Integer());
+				}
 			}
 			break;
 		case SDL_WINDOWEVENT_FOCUS_LOST:
-			if(settings["general"]["enableUiEnhancements"].Bool()) {
-				CCS->musich->setVolume(0);
-				CCS->soundh->setVolume(0);
+			{
+				boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
+				if(settings["general"]["enableUiEnhancements"].Bool()) {
+					CCS->musich->setVolume(0);
+					CCS->soundh->setVolume(0);
+				}
 			}
 			break;
 		}
@@ -171,6 +186,7 @@ void InputHandler::preprocessEvent(const SDL_Event & ev)
 	}
 	else if(ev.type == SDL_SYSWMEVENT)
 	{
+		boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
 		if(!settings["session"]["headless"].Bool() && settings["general"]["notifications"].Bool())
 		{
 			NotificationHandler::handleSdlEvent(ev);
@@ -180,6 +196,7 @@ void InputHandler::preprocessEvent(const SDL_Event & ev)
 	//preprocessing
 	if(ev.type == SDL_MOUSEMOTION)
 	{
+		boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
 		if (CCS && CCS->curh)
 			CCS->curh->cursorMove(ev.motion.x, ev.motion.y);
 	}

client/gui/CGuiHandler.cpp

@@ -95,8 +95,6 @@ void CGuiHandler::handleEvents()
 void CGuiHandler::fakeMouseMove()
 {
 	dispatchMainThread([](){
-		assert(CPlayerInterface::pim);
-		boost::unique_lock lock(*CPlayerInterface::pim);
 		GH.events().dispatchMouseMoved(Point(0, 0), GH.getCursorPosition());
 	});
 }
@@ -114,7 +112,7 @@ void CGuiHandler::stopTextInput()
 void CGuiHandler::renderFrame()
 {
 	{
-		boost::recursive_mutex::scoped_lock un(*CPlayerInterface::pim);
+		boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
 
 		if(nullptr != curInt)
 			curInt->update();

client/gui/CGuiHandler.h

@@ -48,6 +48,8 @@ private:
 	std::unique_ptr<InputHandler> inputHandlerInstance;
 
 public:
+	boost::mutex interfaceMutex;
+
 	/// returns current position of mouse cursor, relative to vcmi window
 	const Point & getCursorPosition() const;
 

client/mainmenu/CMainMenu.cpp

@@ -304,8 +304,6 @@ CMainMenu::CMainMenu()
 
 CMainMenu::~CMainMenu()
 {
-	boost::unique_lock<boost::recursive_mutex> lock(*CPlayerInterface::pim);
-
 	if(GH.curInt == this)
 		GH.curInt = nullptr;
 }

client/mapView/mapHandler.cpp

@@ -15,6 +15,7 @@
 #include "../CCallback.h"
 #include "../CGameInfo.h"
 #include "../CPlayerInterface.h"
+#include "../gui/CGuiHandler.h"
 
 #include "../../lib/CGeneralTextHandler.h"
 #include "../../lib/TerrainHandler.h"
@@ -37,7 +38,7 @@ void CMapHandler::waitForOngoingAnimations()
 {
 	while(CGI->mh->hasOngoingAnimations())
 	{
-		auto unlockPim = vstd::makeUnlockGuard(*CPlayerInterface::pim);
+		auto unlockPim = vstd::makeUnlockGuard(GH.interfaceMutex);
 		boost::this_thread::sleep_for(boost::chrono::milliseconds(1));
 	}
 }

client/windows/settings/GeneralOptionsTab.cpp

@@ -289,7 +289,6 @@ void GeneralOptionsTab::setGameResolution(int index)
 	widget<CLabel>("resolutionLabel")->setText(resolutionToLabelString(resolution.x, resolution.y));
 
 	GH.dispatchMainThread([](){
-		boost::unique_lock<boost::recursive_mutex> lock(*CPlayerInterface::pim);
 		GH.onScreenResize();
 	});
 }
@@ -314,7 +313,6 @@ void GeneralOptionsTab::setFullscreenMode(bool on, bool exclusive)
 	updateResolutionSelector();
 
 	GH.dispatchMainThread([](){
-		boost::unique_lock<boost::recursive_mutex> lock(*CPlayerInterface::pim);
 		GH.onScreenResize();
 	});
 }
@@ -374,7 +372,6 @@ void GeneralOptionsTab::setGameScaling(int index)
 	widget<CLabel>("scalingLabel")->setText(scalingToLabelString(scaling));
 
 	GH.dispatchMainThread([](){
-		boost::unique_lock<boost::recursive_mutex> lock(*CPlayerInterface::pim);
 		GH.onScreenResize();
 	});
 }

docs/players/Cheat_Codes.md

@@ -131,6 +131,5 @@ Below a list of supported commands, with their arguments wrapped in `<>`
 `activate <0/1/2>` - activate game windows (no current use, apparently broken long ago)  
 `redraw` - force full graphical redraw  
 `screen` - show value of screenBuf variable, which prints "screen" when adventure map has current focus, "screen2" otherwise, and dumps values of both screen surfaces to .bmp files  
-`unlock pim` - unlocks specific mutex known in VCMI code as "pim"  
 `not dialog` - set the state indicating if dialog box is active to "no"  
 `tell hs <hero ID> <artifact slot ID>` - write what artifact is present on artifact slot with specified ID for hero with specified ID. (must be called during gameplay)