From d6b9fa8fbd7fe9b9b16822ec74b881b1f8273563 Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Wed, 27 Sep 2023 18:33:52 +0300 Subject: [PATCH 1/4] 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 | 6 +- client/CPlayerInterface.cpp | 20 ++---- client/CPlayerInterface.h | 1 - client/CServerHandler.cpp | 4 +- client/Client.cpp | 11 +--- client/ClientCommandManager.cpp | 19 ++---- client/battle/BattleInterface.cpp | 3 +- client/eventsSDL/InputHandler.cpp | 65 ++++++++++++------- client/gui/CGuiHandler.cpp | 4 +- client/gui/CGuiHandler.h | 2 + client/mainmenu/CMainMenu.cpp | 2 - client/mapView/mapHandler.cpp | 3 +- client/windows/settings/GeneralOptionsTab.cpp | 3 - docs/players/Cheat_Codes.md | 1 - 14 files changed, 65 insertions(+), 79 deletions(-) diff --git a/client/CMT.cpp b/client/CMT.cpp index ee4ae11cc..64b2a4403 100644 --- a/client/CMT.cpp +++ b/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); } diff --git a/client/CPlayerInterface.cpp b/client/CPlayerInterface.cpp index 09bdc174f..2e571bce5 100644 --- a/client/CPlayerInterface.cpp +++ b/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 CPlayerInterface::battleInt; @@ -534,7 +532,6 @@ void CPlayerInterface::garrisonsChanged(ObjectInstanceID id1, ObjectInstanceID i void CPlayerInterface::garrisonsChanged(std::vector objs) { - boost::unique_lock un(*pim); for (auto object : objs) { auto * hero = dynamic_cast(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 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 & components void CPlayerInterface::showYesNoDialog(const std::string &text, CFunctionList onYes, CFunctionList onNo, const std::vector> & components) { - boost::unique_lock 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 &pos) void CPlayerInterface::openHeroWindow(const CGHeroInstance *hero) { - boost::unique_lock un(*pim); GH.windows().createAndPushWindow(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 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); diff --git a/client/CPlayerInterface.h b/client/CPlayerInterface.h index 33651892b..6048e97de 100644 --- a/client/CPlayerInterface.h +++ b/client/CPlayerInterface.h @@ -76,7 +76,6 @@ public: // TODO: make private //minor interfaces CondSh *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 diff --git a/client/CServerHandler.cpp b/client/CServerHandler.cpp index 952b6feb1..5cb875bc1 100644 --- a/client/CServerHandler.cpp +++ b/client/CServerHandler.cpp @@ -90,7 +90,7 @@ template class CApplyOnLobby : public CBaseForLobbyApply public: bool applyOnLobbyHandler(CServerHandler * handler, void * pack) const override { - boost::unique_lock un(*CPlayerInterface::pim); + boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex); T * ptr = static_cast(pack); ApplyOnLobbyHandlerNetPackVisitor visitor(*handler); @@ -296,7 +296,7 @@ void CServerHandler::applyPacksOnLobbyScreen() boost::unique_lock lock(*mx); while(!packsForLobbyScreen.empty()) { - boost::unique_lock 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 diff --git a/client/Client.cpp b/client/Client.cpp index 48343b9e8..0dfa1b80b 100644 --- a/client/Client.cpp +++ b/client/Client.cpp @@ -367,7 +367,6 @@ void CClient::endGame() GH.curInt = nullptr; { - boost::unique_lock 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 gameInterface, PlayerColor color, bool battlecb) { - boost::unique_lock 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 gameInte void CClient::installNewBattleInterface(std::shared_ptr battleInterface, PlayerColor color, bool needCallback) { - boost::unique_lock 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 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 un(*CPlayerInterface::pim); CPlayerInterface::battleInt = std::make_shared(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(playerint[PlayerColor::SPECTATOR]); spectratorInt->cb->onBattleStarted(info); - boost::unique_lock un(*CPlayerInterface::pim); CPlayerInterface::battleInt = std::make_shared(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)); diff --git a/client/ClientCommandManager.cpp b/client/ClientCommandManager.cpp index 068c701d0..f08c9a755 100644 --- a/client/ClientCommandManager.cpp +++ b/client/ClientCommandManager.cpp @@ -74,7 +74,8 @@ void ClientCommandManager::handleGoSoloCommand() { Settings session = settings.write["session"]; - boost::unique_lock 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 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 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(); diff --git a/client/battle/BattleInterface.cpp b/client/battle/BattleInterface.cpp index 484a68be9..53b164184 100644 --- a/client/battle/BattleInterface.cpp +++ b/client/battle/BattleInterface.cpp @@ -106,7 +106,6 @@ void BattleInterface::playIntroSoundAndUnlockInterface() { auto onIntroPlayed = [this]() { - boost::unique_lock 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); } diff --git a/client/eventsSDL/InputHandler.cpp b/client/eventsSDL/InputHandler.cpp index 93524e142..23aa2ea25 100644 --- a/client/eventsSDL/InputHandler.cpp +++ b/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 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 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); } diff --git a/client/gui/CGuiHandler.cpp b/client/gui/CGuiHandler.cpp index ae4f9bdab..1a964cc7b 100644 --- a/client/gui/CGuiHandler.cpp +++ b/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(); diff --git a/client/gui/CGuiHandler.h b/client/gui/CGuiHandler.h index e6d6aa963..20d52f74d 100644 --- a/client/gui/CGuiHandler.h +++ b/client/gui/CGuiHandler.h @@ -48,6 +48,8 @@ private: std::unique_ptr inputHandlerInstance; public: + boost::mutex interfaceMutex; + /// returns current position of mouse cursor, relative to vcmi window const Point & getCursorPosition() const; diff --git a/client/mainmenu/CMainMenu.cpp b/client/mainmenu/CMainMenu.cpp index 34bda33d8..97b672df7 100644 --- a/client/mainmenu/CMainMenu.cpp +++ b/client/mainmenu/CMainMenu.cpp @@ -304,8 +304,6 @@ CMainMenu::CMainMenu() CMainMenu::~CMainMenu() { - boost::unique_lock lock(*CPlayerInterface::pim); - if(GH.curInt == this) GH.curInt = nullptr; } diff --git a/client/mapView/mapHandler.cpp b/client/mapView/mapHandler.cpp index 5b3b197f5..2de09bf59 100644 --- a/client/mapView/mapHandler.cpp +++ b/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)); } } diff --git a/client/windows/settings/GeneralOptionsTab.cpp b/client/windows/settings/GeneralOptionsTab.cpp index f32d568f8..49e490249 100644 --- a/client/windows/settings/GeneralOptionsTab.cpp +++ b/client/windows/settings/GeneralOptionsTab.cpp @@ -289,7 +289,6 @@ void GeneralOptionsTab::setGameResolution(int index) widget("resolutionLabel")->setText(resolutionToLabelString(resolution.x, resolution.y)); GH.dispatchMainThread([](){ - boost::unique_lock lock(*CPlayerInterface::pim); GH.onScreenResize(); }); } @@ -314,7 +313,6 @@ void GeneralOptionsTab::setFullscreenMode(bool on, bool exclusive) updateResolutionSelector(); GH.dispatchMainThread([](){ - boost::unique_lock lock(*CPlayerInterface::pim); GH.onScreenResize(); }); } @@ -374,7 +372,6 @@ void GeneralOptionsTab::setGameScaling(int index) widget("scalingLabel")->setText(scalingToLabelString(scaling)); GH.dispatchMainThread([](){ - boost::unique_lock lock(*CPlayerInterface::pim); GH.onScreenResize(); }); } diff --git a/docs/players/Cheat_Codes.md b/docs/players/Cheat_Codes.md index c54299e5a..e92f674b6 100644 --- a/docs/players/Cheat_Codes.md +++ b/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 ` - write what artifact is present on artifact slot with specified ID for hero with specified ID. (must be called during gameplay) From 0dcfd6e65cd4ef2ed9abbad6e19032c3ce5970cf Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Wed, 27 Sep 2023 18:44:08 +0300 Subject: [PATCH 2/4] Removed optional locking in waitWhileDialog method --- client/CPlayerInterface.cpp | 10 +++++----- client/CPlayerInterface.h | 4 ++-- client/widgets/CArtifactHolder.cpp | 3 ++- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/client/CPlayerInterface.cpp b/client/CPlayerInterface.cpp index 2e571bce5..54ea7a27d 100644 --- a/client/CPlayerInterface.cpp +++ b/client/CPlayerInterface.cpp @@ -1331,7 +1331,7 @@ void CPlayerInterface::showRecruitmentDialog(const CGDwelling *dwelling, const C GH.windows().createAndPushWindow(dwelling, level, dst, recruitCb); } -void CPlayerInterface::waitWhileDialog(bool unlockPim) +void CPlayerInterface::waitWhileDialog() { if (GH.amIGuiThread()) { @@ -1339,7 +1339,7 @@ void CPlayerInterface::waitWhileDialog(bool unlockPim) return; } - auto unlock = vstd::makeUnlockGuardIf(GH.interfaceMutex, unlockPim); + auto unlock = vstd::makeUnlockGuard(GH.interfaceMutex); boost::unique_lock un(showingDialog->mx); while(showingDialog->data) showingDialog->cond.wait(un); @@ -1811,14 +1811,14 @@ void CPlayerInterface::artifactDisassembled(const ArtifactLocation &al) artWin->artifactDisassembled(al); } -void CPlayerInterface::waitForAllDialogs(bool unlockPim) +void CPlayerInterface::waitForAllDialogs() { while(!dialogs.empty()) { - auto unlock = vstd::makeUnlockGuardIf(GH.interfaceMutex, unlockPim); + auto unlock = vstd::makeUnlockGuard(GH.interfaceMutex); boost::this_thread::sleep_for(boost::chrono::milliseconds(5)); } - waitWhileDialog(unlockPim); + waitWhileDialog(); } void CPlayerInterface::proposeLoadingGame() diff --git a/client/CPlayerInterface.h b/client/CPlayerInterface.h index 6048e97de..fdcad9ff2 100644 --- a/client/CPlayerInterface.h +++ b/client/CPlayerInterface.h @@ -184,8 +184,8 @@ public: // public interface for use by client via LOCPLINT access void showHeroExchange(ObjectInstanceID hero1, ObjectInstanceID hero2); void showArtifactAssemblyDialog(const Artifact * artifact, const Artifact * assembledArtifact, CFunctionList onYes); - void waitWhileDialog(bool unlockPim = true); - void waitForAllDialogs(bool unlockPim = true); + void waitWhileDialog(); + void waitForAllDialogs(); void openTownWindow(const CGTownInstance * town); //shows townscreen void openHeroWindow(const CGHeroInstance * hero); //shows hero window with given hero diff --git a/client/widgets/CArtifactHolder.cpp b/client/widgets/CArtifactHolder.cpp index b73b6b795..f4cd8e979 100644 --- a/client/widgets/CArtifactHolder.cpp +++ b/client/widgets/CArtifactHolder.cpp @@ -287,6 +287,7 @@ bool ArtifactUtilsClient::askToAssemble(const CGHeroInstance * hero, const Artif { auto askThread = new boost::thread([hero, art, slot, assemblyPossibilities]() -> void { + boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex); for(const auto combinedArt : assemblyPossibilities) { bool assembleConfirmed = false; @@ -294,7 +295,7 @@ bool ArtifactUtilsClient::askToAssemble(const CGHeroInstance * hero, const Artif onYesHandlers += std::bind(&CCallback::assembleArtifacts, LOCPLINT->cb.get(), hero, slot, true, combinedArt->getId()); LOCPLINT->showArtifactAssemblyDialog(art->artType, combinedArt, onYesHandlers); - LOCPLINT->waitWhileDialog(false); + LOCPLINT->waitWhileDialog(); if(assembleConfirmed) break; } From 195320dcf2ea608b4263885fced730a64440922f Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Wed, 27 Sep 2023 18:49:30 +0300 Subject: [PATCH 3/4] Removed remaining references to 'pim' name --- client/CMT.cpp | 2 +- client/CPlayerInterface.cpp | 10 +++++----- client/Client.cpp | 14 ++++++++------ client/ClientCommandManager.h | 3 --- client/battle/BattleInterface.cpp | 2 +- client/mapView/mapHandler.cpp | 2 +- 6 files changed, 16 insertions(+), 17 deletions(-) diff --git a/client/CMT.cpp b/client/CMT.cpp index 64b2a4403..84d808dc5 100644 --- a/client/CMT.cpp +++ b/client/CMT.cpp @@ -486,7 +486,7 @@ 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() + // unlock it here to avoid assertion failure on GH destruction in exit() GH.interfaceMutex.unlock(); exit(0); } diff --git a/client/CPlayerInterface.cpp b/client/CPlayerInterface.cpp index 54ea7a27d..6d03659f7 100644 --- a/client/CPlayerInterface.cpp +++ b/client/CPlayerInterface.cpp @@ -104,7 +104,7 @@ #include "../lib/spells/CSpellHandler.h" // The macro below is used to mark functions that are called by client when game state changes. -// They all assume that CPlayerInterface::pim mutex is locked. +// They all assume that interface mutex is locked. #define EVENT_HANDLER_CALLED_BY_CLIENT #define BATTLE_EVENT_POSSIBLE_RETURN \ @@ -751,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(GH.interfaceMutex); + auto unlockInterface = vstd::makeUnlockGuard(GH.interfaceMutex); autofightingAI->activeStack(battleID, stack); return; } @@ -1339,7 +1339,7 @@ void CPlayerInterface::waitWhileDialog() return; } - auto unlock = vstd::makeUnlockGuard(GH.interfaceMutex); + auto unlockInterface = vstd::makeUnlockGuard(GH.interfaceMutex); boost::unique_lock un(showingDialog->mx); while(showingDialog->data) showingDialog->cond.wait(un); @@ -1378,7 +1378,7 @@ void CPlayerInterface::centerView (int3 pos, int focusTime) GH.windows().totalRedraw(); { IgnoreEvents ignore(*this); - auto unlockPim = vstd::makeUnlockGuard(GH.interfaceMutex); + auto unlockInterface = vstd::makeUnlockGuard(GH.interfaceMutex); boost::this_thread::sleep_for(boost::chrono::milliseconds(focusTime)); } } @@ -1815,7 +1815,7 @@ void CPlayerInterface::waitForAllDialogs() { while(!dialogs.empty()) { - auto unlock = vstd::makeUnlockGuard(GH.interfaceMutex); + auto unlockInterface = vstd::makeUnlockGuard(GH.interfaceMutex); boost::this_thread::sleep_for(boost::chrono::milliseconds(5)); } waitWhileDialog(); diff --git a/client/Client.cpp b/client/Client.cpp index 0dfa1b80b..cfc036fed 100644 --- a/client/Client.cpp +++ b/client/Client.cpp @@ -641,15 +641,17 @@ void CClient::battleFinished(const BattleID & battleID) void CClient::startPlayerBattleAction(const BattleID & battleID, PlayerColor color) { - assert(vstd::contains(battleints, color)); + auto battleint = battleints.at(color); - if(vstd::contains(battleints, color)) + if (!battleint->human) { // we want to avoid locking gamestate and causing UI to freeze while AI is making turn - 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)); + auto unlockInterface = vstd::makeUnlockGuard(GH.interfaceMutex); + battleint->activeStack(battleID, gs->getBattle(battleID)->battleGetStackByID(gs->getBattle(battleID)->activeStack, false)); + } + else + { + battleint->activeStack(battleID, gs->getBattle(battleID)->battleGetStackByID(gs->getBattle(battleID)->activeStack, false)); } } diff --git a/client/ClientCommandManager.h b/client/ClientCommandManager.h index 409ccc6ff..ecb555212 100644 --- a/client/ClientCommandManager.h +++ b/client/ClientCommandManager.h @@ -78,9 +78,6 @@ class ClientCommandManager //take mantis #2292 issue about account if thinking a // set - sets special temporary settings that reset on game quit. void handleSetCommand(std::istringstream& singleWordBuffer); - // Unlocks specific mutex known in VCMI code as "pim" - void handleUnlockCommand(std::istringstream& singleWordBuffer); - // Crashes the game forcing an exception void handleCrashCommand(); diff --git a/client/battle/BattleInterface.cpp b/client/battle/BattleInterface.cpp index 53b164184..b0e383a66 100644 --- a/client/battle/BattleInterface.cpp +++ b/client/battle/BattleInterface.cpp @@ -780,7 +780,7 @@ void BattleInterface::onAnimationsFinished() void BattleInterface::waitForAnimations() { { - auto unlockPim = vstd::makeUnlockGuard(GH.interfaceMutex); + auto unlockInterface = vstd::makeUnlockGuard(GH.interfaceMutex); ongoingAnimationsState.waitUntil(false); } diff --git a/client/mapView/mapHandler.cpp b/client/mapView/mapHandler.cpp index 2de09bf59..2b83b2fba 100644 --- a/client/mapView/mapHandler.cpp +++ b/client/mapView/mapHandler.cpp @@ -38,7 +38,7 @@ void CMapHandler::waitForOngoingAnimations() { while(CGI->mh->hasOngoingAnimations()) { - auto unlockPim = vstd::makeUnlockGuard(GH.interfaceMutex); + auto unlockInterface = vstd::makeUnlockGuard(GH.interfaceMutex); boost::this_thread::sleep_for(boost::chrono::milliseconds(1)); } } From 03a939fd521ecbc38b19566463ee6c2dda3d5e81 Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Wed, 27 Sep 2023 19:13:26 +0300 Subject: [PATCH 4/4] Remove redundant thread name - this method is not thread entry point --- client/CServerHandler.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/client/CServerHandler.cpp b/client/CServerHandler.cpp index 5cb875bc1..e486a9351 100644 --- a/client/CServerHandler.cpp +++ b/client/CServerHandler.cpp @@ -611,8 +611,6 @@ void CServerHandler::startMapAfterConnection(std::shared_ptr to) void CServerHandler::startGameplay(VCMI_LIB_WRAP_NAMESPACE(CGameState) * gameState) { - setThreadName("startGameplay"); - if(CMM) CMM->disable(); client = new CClient();