From a81f18765c119a4c11c24e121f187d61dedc6324 Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Sun, 6 Aug 2023 12:45:20 +0300 Subject: [PATCH 01/20] Another workaround for another crash on Android launcher --- .../src/main/java/eu/vcmi/vcmi/util/FileUtil.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/android/vcmi-app/src/main/java/eu/vcmi/vcmi/util/FileUtil.java b/android/vcmi-app/src/main/java/eu/vcmi/vcmi/util/FileUtil.java index e64dd3afa..49b731425 100644 --- a/android/vcmi-app/src/main/java/eu/vcmi/vcmi/util/FileUtil.java +++ b/android/vcmi-app/src/main/java/eu/vcmi/vcmi/util/FileUtil.java @@ -81,7 +81,7 @@ public class FileUtil { if (file == null) { - Log.e("Broken path given to fileutil"); + Log.e("Broken path given to fileutil::ensureWriteable"); return false; } @@ -99,6 +99,12 @@ public class FileUtil public static boolean clearDirectory(final File dir) { + if (dir == null) + { + Log.e("Broken path given to fileutil::clearDirectory"); + return false; + } + for (final File f : dir.listFiles()) { if (f.isDirectory() && !clearDirectory(f)) From 0dca5c45bd90f3b6e48a0abbe49f46dd8cc1bc42 Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Sun, 6 Aug 2023 12:46:29 +0300 Subject: [PATCH 02/20] Workaround for potential crash on opening town dwelling with no valid creatures --- client/windows/CCastleInterface.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/client/windows/CCastleInterface.cpp b/client/windows/CCastleInterface.cpp index dda745cea..c5a1327bf 100644 --- a/client/windows/CCastleInterface.cpp +++ b/client/windows/CCastleInterface.cpp @@ -849,7 +849,13 @@ void CCastleBuildings::enterCastleGate() void CCastleBuildings::enterDwelling(int level) { - assert(level >= 0 && level < town->creatures.size()); + if (level < 0 || level >= town->creatures.size() || town->creatures[level].second.empty()) + { + assert(0); + logGlobal->error("Attempt to enter into invalid dwelling of level %d in town %s (%s)", level, town->getNameTranslated(), town->town->faction->getNameTranslated()); + return; + } + auto recruitCb = [=](CreatureID id, int count) { LOCPLINT->cb->recruitCreatures(town, town->getUpperArmy(), id, count, level); From a4ff408c9decc0038cfa1206b35c219b62edd0fb Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Sun, 6 Aug 2023 13:29:57 +0300 Subject: [PATCH 03/20] Removed unused code --- client/render/Graphics.cpp | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/client/render/Graphics.cpp b/client/render/Graphics.cpp index bb9e385e9..6212b357b 100644 --- a/client/render/Graphics.cpp +++ b/client/render/Graphics.cpp @@ -126,24 +126,11 @@ void Graphics::initializeBattleGraphics() } Graphics::Graphics() { - #if 0 - - std::vector<Task> tasks; //preparing list of graphics to load - tasks += std::bind(&Graphics::loadFonts,this); - tasks += std::bind(&Graphics::loadPaletteAndColors,this); - tasks += std::bind(&Graphics::initializeBattleGraphics,this); - tasks += std::bind(&Graphics::loadErmuToPicture,this); - tasks += std::bind(&Graphics::initializeImageLists,this); - - CThreadHelper th(&tasks,std::max((ui32)1,boost::thread::hardware_concurrency())); - th.run(); - #else loadFonts(); loadPaletteAndColors(); initializeBattleGraphics(); loadErmuToPicture(); initializeImageLists(); - #endif //(!) do not load any CAnimation here } From 8b0f9c86f886f6a0c96b923c320f6ba164f12ce2 Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Sun, 6 Aug 2023 13:30:20 +0300 Subject: [PATCH 04/20] Fixed possible crash on invalid town building name --- lib/CTownHandler.cpp | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/CTownHandler.cpp b/lib/CTownHandler.cpp index f756c79de..ee47103db 100644 --- a/lib/CTownHandler.cpp +++ b/lib/CTownHandler.cpp @@ -1183,11 +1183,19 @@ void CTownHandler::initializeRequirements() { if (node.Vector().size() > 1) { - logMod->warn("Unexpected length of town buildings requirements: %d", node.Vector().size()); - logMod->warn("Entry contains: "); - logMod->warn(node.toJson()); + logMod->error("Unexpected length of town buildings requirements: %d", node.Vector().size()); + logMod->error("Entry contains: "); + logMod->error(node.toJson()); } - return BuildingID(VLC->modh->identifiers.getIdentifier(requirement.town->getBuildingScope(), node.Vector()[0]).value()); + + auto index = VLC->modh->identifiers.getIdentifier(requirement.town->getBuildingScope(), node[0]); + + if (!index.has_value()) + { + logMod->error("Unknown building in town buildings: %s", node[0].String()); + return BuildingID::NONE; + } + return BuildingID(index.value()); }); } requirementsToLoad.clear(); From 5faaf175f9cc97042b18745623ef54e7a9fd84e6 Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Sun, 6 Aug 2023 13:31:10 +0300 Subject: [PATCH 05/20] Added debug information to SDL-related crashes --- client/CMT.cpp | 11 +++++++++++ client/CMT.h | 4 ++++ client/renderSDL/SDL_Extensions.cpp | 11 +++++++++++ client/renderSDL/ScreenHandler.cpp | 10 +++++++++- 4 files changed, 35 insertions(+), 1 deletion(-) diff --git a/client/CMT.cpp b/client/CMT.cpp index 3f4fe83b4..4e050afab 100644 --- a/client/CMT.cpp +++ b/client/CMT.cpp @@ -38,6 +38,7 @@ #include <vstd/StringUtils.h> #include <SDL_main.h> +#include <SDL.h> #ifdef VCMI_ANDROID #include "../lib/CAndroidVMHelper.h" @@ -510,3 +511,13 @@ void handleQuit(bool ask) quitApplication(); } } + +void handleFatalError(const std::string & message) +{ + logGlobal->error("FATAL ERROR ENCOUTERED, VCMI WILL NOW TERMINATE"); + logGlobal->error("Reason: %s", message); + + SDL_ShowSimpleMessageBox(SDL_MESSAGEBOX_ERROR, "Fatal error", message.c_str(), nullptr); + + throw std::runtime_error(message); +} diff --git a/client/CMT.h b/client/CMT.h index 887b704c9..cfea318ff 100644 --- a/client/CMT.h +++ b/client/CMT.h @@ -21,3 +21,7 @@ extern SDL_Surface *screen2; // and hlp surface (used to store not-active in extern SDL_Surface *screenBuf; // points to screen (if only advmapint is present) or screen2 (else) - should be used when updating controls which are not regularly redrawed void handleQuit(bool ask = true); + +/// Notify user about encoutered fatal error and terminate the game +/// TODO: decide on better location for this method +[[noreturn]] void handleFatalError(const std::string & message); diff --git a/client/renderSDL/SDL_Extensions.cpp b/client/renderSDL/SDL_Extensions.cpp index b362e137f..3d6323cd5 100644 --- a/client/renderSDL/SDL_Extensions.cpp +++ b/client/renderSDL/SDL_Extensions.cpp @@ -80,6 +80,17 @@ SDL_Surface * CSDL_Ext::newSurface(int w, int h) SDL_Surface * CSDL_Ext::newSurface(int w, int h, SDL_Surface * mod) //creates new surface, with flags/format same as in surface given { SDL_Surface * ret = SDL_CreateRGBSurface(0,w,h,mod->format->BitsPerPixel,mod->format->Rmask,mod->format->Gmask,mod->format->Bmask,mod->format->Amask); + + if(ret == nullptr) + { + const char * error = SDL_GetError(); + + std::string messagePattern = "Failed to create SDL Surface of size %d x %d, %d bpp. Reason: %s"; + std::string message = boost::str(boost::format(messagePattern) % w % h % mod->format->BitsPerPixel % error); + + handleFatalError(message); + } + if (mod->format->palette) { assert(ret->format->palette); diff --git a/client/renderSDL/ScreenHandler.cpp b/client/renderSDL/ScreenHandler.cpp index 8de235d47..8f071cbe1 100644 --- a/client/renderSDL/ScreenHandler.cpp +++ b/client/renderSDL/ScreenHandler.cpp @@ -264,7 +264,15 @@ void ScreenHandler::initializeWindow() mainWindow = createWindow(); if(mainWindow == nullptr) - throw std::runtime_error("Unable to create window\n"); + { + const char * error = SDL_GetError(); + Point dimensions = getPreferredWindowResolution(); + + std::string messagePattern = "Failed to create SDL Window of size %d x %d. Reason: %s"; + std::string message = boost::str(boost::format(messagePattern) % dimensions.x % dimensions.y % error); + + handleFatalError(message); + } //create first available renderer if preferred not set. Use no flags, so HW accelerated will be preferred but SW renderer also will possible mainRenderer = SDL_CreateRenderer(mainWindow, getPreferredRenderingDriver(), 0); From 702f57d5cc93d4e07dff51ad976c7c9fba654ee4 Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Sun, 6 Aug 2023 13:43:08 +0300 Subject: [PATCH 06/20] Avoid another crash in Android launcher --- .../java/eu/vcmi/vcmi/settings/ExportDataController.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/android/vcmi-app/src/main/java/eu/vcmi/vcmi/settings/ExportDataController.java b/android/vcmi-app/src/main/java/eu/vcmi/vcmi/settings/ExportDataController.java index b799d10c1..35fdb6e2d 100644 --- a/android/vcmi-app/src/main/java/eu/vcmi/vcmi/settings/ExportDataController.java +++ b/android/vcmi-app/src/main/java/eu/vcmi/vcmi/settings/ExportDataController.java @@ -133,6 +133,12 @@ public class ExportDataController extends LauncherSettingController<Void, Void> } } + if (exported == null) + { + publishProgress("Failed to copy file " + child.getName()); + return false; + } + try( final OutputStream targetStream = owner.getContentResolver() .openOutputStream(exported.getUri()); From 48f130cd763b7ff4b3034d570545ee063abb30a5 Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Sun, 6 Aug 2023 14:11:14 +0300 Subject: [PATCH 07/20] Fix crash on haptic feedback on Android --- android/vcmi-app/src/main/java/eu/vcmi/vcmi/NativeMethods.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/android/vcmi-app/src/main/java/eu/vcmi/vcmi/NativeMethods.java b/android/vcmi-app/src/main/java/eu/vcmi/vcmi/NativeMethods.java index 8470e2d16..55ca15691 100644 --- a/android/vcmi-app/src/main/java/eu/vcmi/vcmi/NativeMethods.java +++ b/android/vcmi-app/src/main/java/eu/vcmi/vcmi/NativeMethods.java @@ -146,7 +146,7 @@ public class NativeMethods public static void hapticFeedback() { final Context ctx = SDL.getContext(); - if (Build.VERSION.SDK_INT >= 26) { + if (Build.VERSION.SDK_INT >= 29) { ((Vibrator) ctx.getSystemService(ctx.VIBRATOR_SERVICE)).vibrate(VibrationEffect.createPredefined(VibrationEffect.EFFECT_TICK)); } else { ((Vibrator) ctx.getSystemService(ctx.VIBRATOR_SERVICE)).vibrate(30); From 0c9e2d6fddfa8aebb2205074f8ae203f86892baa Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Sun, 6 Aug 2023 14:11:31 +0300 Subject: [PATCH 08/20] Workaround for crash on map rendering --- client/mapView/MapRenderer.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/client/mapView/MapRenderer.cpp b/client/mapView/MapRenderer.cpp index a9f8b63b6..0b073177a 100644 --- a/client/mapView/MapRenderer.cpp +++ b/client/mapView/MapRenderer.cpp @@ -523,6 +523,14 @@ uint8_t MapRendererObjects::checksum(IMapRendererContext & context, const int3 & for(const auto & objectID : context.getObjects(coordinates)) { const auto * objectInstance = context.getObject(objectID); + + assert(objectInstance); + if(!objectInstance) + { + logGlobal->error("Stray map object that isn't fading"); + continue; + } + size_t groupIndex = context.objectGroupIndex(objectInstance->id); Point offsetPixels = context.objectImageOffset(objectInstance->id, coordinates); From 9e4a42b25df2bf2abbe6c587c3434ac1dfe15d98 Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Sun, 6 Aug 2023 14:33:20 +0300 Subject: [PATCH 09/20] Android build ID bump --- android/vcmi-app/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/android/vcmi-app/build.gradle b/android/vcmi-app/build.gradle index 1d818d14a..226f435f3 100644 --- a/android/vcmi-app/build.gradle +++ b/android/vcmi-app/build.gradle @@ -10,7 +10,7 @@ android { applicationId "is.xyz.vcmi" minSdk 19 targetSdk 31 - versionCode 1302 + versionCode 1303 versionName "1.3.0" setProperty("archivesBaseName", "vcmi") } From c524caee5c9055fda30c95a6b030402a10d4f290 Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Sun, 6 Aug 2023 17:46:49 +0300 Subject: [PATCH 10/20] Fixed possible uncaught exception on starting map --- client/CServerHandler.cpp | 13 +++++++++++-- client/CServerHandler.h | 2 +- lib/StartInfo.cpp | 6 +++--- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/client/CServerHandler.cpp b/client/CServerHandler.cpp index 225c09c9e..18e9ee444 100644 --- a/client/CServerHandler.cpp +++ b/client/CServerHandler.cpp @@ -572,7 +572,16 @@ void CServerHandler::sendRestartGame() const void CServerHandler::sendStartGame(bool allowOnlyAI) const { - verifyStateBeforeStart(allowOnlyAI ? true : settings["session"]["onlyai"].Bool()); + try + { + verifyStateBeforeStart(allowOnlyAI ? true : settings["session"]["onlyai"].Bool()); + } + catch (const std::exception & e) + { + showServerError( std::string("Unable to start map! Reason: ") + e.what()); + return; + } + LobbyStartGame lsg; if(client) { @@ -696,7 +705,7 @@ void CServerHandler::startCampaignScenario(std::shared_ptr<CampaignState> cs) }); } -void CServerHandler::showServerError(std::string txt) +void CServerHandler::showServerError(std::string txt) const { CInfoWindow::showInfoDialog(txt, {}); } diff --git a/client/CServerHandler.h b/client/CServerHandler.h index 199aa04a8..4b3e61c65 100644 --- a/client/CServerHandler.h +++ b/client/CServerHandler.h @@ -151,7 +151,7 @@ public: void startGameplay(VCMI_LIB_WRAP_NAMESPACE(CGameState) * gameState = nullptr); void endGameplay(bool closeConnection = true, bool restart = false); void startCampaignScenario(std::shared_ptr<CampaignState> cs = {}); - void showServerError(std::string txt); + void showServerError(std::string txt) const; // TODO: LobbyState must be updated within game so we should always know how many player interfaces our client handle int howManyPlayerInterfaces(); diff --git a/lib/StartInfo.cpp b/lib/StartInfo.cpp index c722c80d5..6338a1541 100644 --- a/lib/StartInfo.cpp +++ b/lib/StartInfo.cpp @@ -71,7 +71,7 @@ std::string StartInfo::getCampaignName() const void LobbyInfo::verifyStateBeforeStart(bool ignoreNoHuman) const { if(!mi || !mi->mapHeader) - throw std::domain_error("ExceptionMapMissing"); + throw std::domain_error("There is no map to start!"); auto missingMods = CMapService::verifyMapHeaderMods(*mi->mapHeader); CModHandler::Incompatibility::ModList modList; @@ -88,12 +88,12 @@ void LobbyInfo::verifyStateBeforeStart(bool ignoreNoHuman) const break; if(i == si->playerInfos.cend() && !ignoreNoHuman) - throw std::domain_error("ExceptionNoHuman"); + throw std::domain_error("There is no human player on map"); if(si->mapGenOptions && si->mode == StartInfo::NEW_GAME) { if(!si->mapGenOptions->checkOptions()) - throw std::domain_error("ExceptionNoTemplate"); + throw std::domain_error("No random map template found!"); } } From 6ddac8376d9c06ee580f0c95dc3bc24ee988ef5e Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Sun, 6 Aug 2023 17:47:12 +0300 Subject: [PATCH 11/20] Fixed possible crash on attempt to load missing font(?) --- client/renderSDL/CBitmapFont.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/client/renderSDL/CBitmapFont.cpp b/client/renderSDL/CBitmapFont.cpp index 0810b649b..fd4be26c1 100644 --- a/client/renderSDL/CBitmapFont.cpp +++ b/client/renderSDL/CBitmapFont.cpp @@ -26,6 +26,12 @@ void CBitmapFont::loadModFont(const std::string & modName, const ResourceID & resource) { + if (!CResourceHandler::get(modName)->existsResource(resource)) + { + logGlobal->error("Failed to load font %s from mod %s", resource.getName(), modName); + return; + } + auto data = CResourceHandler::get(modName)->load(resource)->readAll(); std::string modLanguage = CGI->modh->getModLanguage(modName); std::string modEncoding = Languages::getLanguageOptions(modLanguage).encoding; From 0b99fc0ccb992c6a2e162242e1ee66505220d1e6 Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Sun, 6 Aug 2023 18:12:36 +0300 Subject: [PATCH 12/20] Show message box when H3 data is missing --- client/CMT.cpp | 26 ++++++++++++-------------- client/CMT.h | 2 +- client/renderSDL/SDL_Extensions.cpp | 2 +- client/renderSDL/ScreenHandler.cpp | 2 +- 4 files changed, 15 insertions(+), 17 deletions(-) diff --git a/client/CMT.cpp b/client/CMT.cpp index 4e050afab..b744aee4c 100644 --- a/client/CMT.cpp +++ b/client/CMT.cpp @@ -261,19 +261,12 @@ int main(int argc, char * argv[]) if (CResourceHandler::get()->existsResource(ResourceID(filename))) return true; - logGlobal->error("Error: %s was not found!", message); - return false; + handleFatalError(message, false); }; - if (!testFile("DATA/HELP.TXT", "Heroes III data") || - !testFile("MODS/VCMI/MOD.JSON", "VCMI data")) - { - exit(1); // These are unrecoverable errors - } - - // these two are optional + some installs have them on CD and not in data directory - testFile("VIDEO/GOOD1A.SMK", "campaign movies"); - testFile("SOUNDS/G1A.WAV", "campaign music"); //technically not a music but voiced intro sounds + testFile("DATA/HELP.TXT", "VCMI requires Heroes III: Shadow of Death or Heroes III: Complete data files to run!"); + testFile("MODS/VCMI/MOD.JSON", "VCMI installation is corrupted! Built-in mod was not found!"); + testFile("DATA/TENTCOLR.TXT", "Heroes III: Restoration of Erathia (including HD Edition) data files are not supported!"); srand ( (unsigned int)time(nullptr) ); @@ -512,12 +505,17 @@ void handleQuit(bool ask) } } -void handleFatalError(const std::string & message) +void handleFatalError(const std::string & message, bool terminate) { logGlobal->error("FATAL ERROR ENCOUTERED, VCMI WILL NOW TERMINATE"); logGlobal->error("Reason: %s", message); - SDL_ShowSimpleMessageBox(SDL_MESSAGEBOX_ERROR, "Fatal error", message.c_str(), nullptr); + std::string messageToShow = "Fatal error! " + message; - throw std::runtime_error(message); + SDL_ShowSimpleMessageBox(SDL_MESSAGEBOX_ERROR, "Fatal error!", messageToShow.c_str(), nullptr); + + if (terminate) + throw std::runtime_error(message); + else + exit(1); } diff --git a/client/CMT.h b/client/CMT.h index cfea318ff..28d877875 100644 --- a/client/CMT.h +++ b/client/CMT.h @@ -24,4 +24,4 @@ void handleQuit(bool ask = true); /// Notify user about encoutered fatal error and terminate the game /// TODO: decide on better location for this method -[[noreturn]] void handleFatalError(const std::string & message); +[[noreturn]] void handleFatalError(const std::string & message, bool terminate); diff --git a/client/renderSDL/SDL_Extensions.cpp b/client/renderSDL/SDL_Extensions.cpp index 3d6323cd5..b4d7f1249 100644 --- a/client/renderSDL/SDL_Extensions.cpp +++ b/client/renderSDL/SDL_Extensions.cpp @@ -88,7 +88,7 @@ SDL_Surface * CSDL_Ext::newSurface(int w, int h, SDL_Surface * mod) //creates ne std::string messagePattern = "Failed to create SDL Surface of size %d x %d, %d bpp. Reason: %s"; std::string message = boost::str(boost::format(messagePattern) % w % h % mod->format->BitsPerPixel % error); - handleFatalError(message); + handleFatalError(message, true); } if (mod->format->palette) diff --git a/client/renderSDL/ScreenHandler.cpp b/client/renderSDL/ScreenHandler.cpp index 8f071cbe1..623289074 100644 --- a/client/renderSDL/ScreenHandler.cpp +++ b/client/renderSDL/ScreenHandler.cpp @@ -271,7 +271,7 @@ void ScreenHandler::initializeWindow() std::string messagePattern = "Failed to create SDL Window of size %d x %d. Reason: %s"; std::string message = boost::str(boost::format(messagePattern) % dimensions.x % dimensions.y % error); - handleFatalError(message); + handleFatalError(message, true); } //create first available renderer if preferred not set. Use no flags, so HW accelerated will be preferred but SW renderer also will possible From b5fc0f19f23a7725602723b4c302d4b9f095a309 Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Mon, 7 Aug 2023 16:53:51 +0300 Subject: [PATCH 13/20] Proper fix for Android launcher nullptr dereference --- android/vcmi-app/src/main/java/eu/vcmi/vcmi/util/FileUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/android/vcmi-app/src/main/java/eu/vcmi/vcmi/util/FileUtil.java b/android/vcmi-app/src/main/java/eu/vcmi/vcmi/util/FileUtil.java index 49b731425..887c8ba9c 100644 --- a/android/vcmi-app/src/main/java/eu/vcmi/vcmi/util/FileUtil.java +++ b/android/vcmi-app/src/main/java/eu/vcmi/vcmi/util/FileUtil.java @@ -99,7 +99,7 @@ public class FileUtil public static boolean clearDirectory(final File dir) { - if (dir == null) + if (dir == null || dir.listFiles() == null) { Log.e("Broken path given to fileutil::clearDirectory"); return false; From b167aeafd850b42d667a0728b017194da8032f3b Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Mon, 7 Aug 2023 17:03:19 +0300 Subject: [PATCH 14/20] Build ID bump for Android --- android/vcmi-app/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/android/vcmi-app/build.gradle b/android/vcmi-app/build.gradle index 226f435f3..0a00e9de0 100644 --- a/android/vcmi-app/build.gradle +++ b/android/vcmi-app/build.gradle @@ -10,7 +10,7 @@ android { applicationId "is.xyz.vcmi" minSdk 19 targetSdk 31 - versionCode 1303 + versionCode 1304 versionName "1.3.0" setProperty("archivesBaseName", "vcmi") } From d7cbe4ecde8ae8fb56350886fb00db70ff8c17b2 Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Mon, 7 Aug 2023 17:03:39 +0300 Subject: [PATCH 15/20] Attempt to fix data raced in battle on server --- server/CGameHandler.cpp | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/server/CGameHandler.cpp b/server/CGameHandler.cpp index 5b6385ac0..3cb0f5655 100644 --- a/server/CGameHandler.cpp +++ b/server/CGameHandler.cpp @@ -40,6 +40,7 @@ #include "../lib/CCreatureHandler.h" #include "../lib/gameState/CGameState.h" #include "../lib/CStack.h" +#include "../lib/UnlockGuard.h" #include "../lib/GameSettings.h" #include "../lib/battle/BattleInfo.h" #include "../lib/CondSh.h" @@ -76,6 +77,7 @@ #define COMPLAIN_RETF(txt, FORMAT) {complain(boost::str(boost::format(txt) % FORMAT)); return false;} CondSh<bool> battleMadeAction(false); +boost::recursive_mutex battleActionMutex; CondSh<BattleResult *> battleResult(nullptr); template <typename T> class CApplyOnGH; @@ -4394,6 +4396,8 @@ void CGameHandler::updateGateState() bool CGameHandler::makeBattleAction(BattleAction &ba) { + boost::unique_lock lock(battleActionMutex); + bool ok = true; battle::Target target = ba.getTarget(gs->curB); @@ -4817,6 +4821,8 @@ bool CGameHandler::makeBattleAction(BattleAction &ba) bool CGameHandler::makeCustomAction(BattleAction & ba) { + boost::unique_lock lock(battleActionMutex); + switch(ba.actionType) { case EActionType::HERO_SPELL: @@ -6048,6 +6054,8 @@ bool CGameHandler::swapStacks(const StackLocation & sl1, const StackLocation & s void CGameHandler::runBattle() { + boost::unique_lock lock(battleActionMutex); + setBattle(gs->curB); assert(gs->curB); //TODO: pre-tactic stuff, call scripts etc. @@ -6066,7 +6074,10 @@ void CGameHandler::runBattle() //tactic round { while ((lobby->state != EServerState::SHUTDOWN) && gs->curB->tacticDistance && !battleResult.get()) + { + auto unlockGuard = vstd::makeUnlockGuard(battleActionMutex); boost::this_thread::sleep(boost::posix_time::milliseconds(50)); + } } //initial stacks appearance triggers, e.g. built-in bonus spells @@ -6389,7 +6400,10 @@ void CGameHandler::runBattle() battleMadeAction.data = false; while ((lobby->state != EServerState::SHUTDOWN) && !actionWasMade()) { - battleMadeAction.cond.wait(lock); + { + auto unlockGuard = vstd::makeUnlockGuard(battleActionMutex); + battleMadeAction.cond.wait(lock); + } if (battleGetStackByID(nextId, false) != next) next = nullptr; //it may be removed, while we wait } From 04fe78d31cd915d58a7b85dcaa6ee62a57e32111 Mon Sep 17 00:00:00 2001 From: Andrii Danylchenko <danilchenko.andrij@gmail.com> Date: Sat, 5 Aug 2023 13:49:49 +0300 Subject: [PATCH 16/20] NKAI: fix freeze on army gathering --- .../Behaviors/GatherArmyBehavior.cpp | 28 +++++++++---------- AI/Nullkiller/Engine/Nullkiller.cpp | 5 ++++ AI/VCAI/VCAI.cpp | 2 +- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/AI/Nullkiller/Behaviors/GatherArmyBehavior.cpp b/AI/Nullkiller/Behaviors/GatherArmyBehavior.cpp index f5eb28c79..c73b374c0 100644 --- a/AI/Nullkiller/Behaviors/GatherArmyBehavior.cpp +++ b/AI/Nullkiller/Behaviors/GatherArmyBehavior.cpp @@ -92,15 +92,6 @@ Goals::TGoalVec GatherArmyBehavior::deliverArmyToHero(const CGHeroInstance * her continue; } - bool garrisoned = false; - - if(path.turn() == 0 && hero->inTownGarrison) - { -#if NKAI_TRACE_LEVEL >= 1 - garrisoned = true; -#endif - } - if(path.turn() > 0 && ai->nullkiller->dangerHitMap->enemyCanKillOurHeroesAlongThePath(path)) { #if NKAI_TRACE_LEVEL >= 2 @@ -184,15 +175,22 @@ Goals::TGoalVec GatherArmyBehavior::deliverArmyToHero(const CGHeroInstance * her composition.addNext(heroExchange); - if(garrisoned && path.turn() == 0) + if(hero->inTownGarrison && path.turn() == 0) { auto lockReason = ai->nullkiller->getHeroLockedReason(hero); - composition.addNextSequence({ - sptr(ExchangeSwapTownHeroes(hero->visitedTown)), - sptr(exchangePath), - sptr(ExchangeSwapTownHeroes(hero->visitedTown, hero, lockReason)) - }); + if(path.targetHero->visitedTown == hero->visitedTown) + { + composition.addNextSequence({ + sptr(ExchangeSwapTownHeroes(hero->visitedTown, hero, lockReason))}); + } + else + { + composition.addNextSequence({ + sptr(ExchangeSwapTownHeroes(hero->visitedTown)), + sptr(exchangePath), + sptr(ExchangeSwapTownHeroes(hero->visitedTown, hero, lockReason))}); + } } else { diff --git a/AI/Nullkiller/Engine/Nullkiller.cpp b/AI/Nullkiller/Engine/Nullkiller.cpp index 66b28ca8e..d6d7f41dc 100644 --- a/AI/Nullkiller/Engine/Nullkiller.cpp +++ b/AI/Nullkiller/Engine/Nullkiller.cpp @@ -323,6 +323,11 @@ void Nullkiller::makeTurn() } executeTask(bestTask); + + if(i == MAXPASS) + { + logAi->error("Goal %s exceeded maxpass. Terminating AI turn.", bestTask->toString()); + } } } diff --git a/AI/VCAI/VCAI.cpp b/AI/VCAI/VCAI.cpp index c3bd49b6d..f20ceda1f 100644 --- a/AI/VCAI/VCAI.cpp +++ b/AI/VCAI/VCAI.cpp @@ -1359,7 +1359,7 @@ void VCAI::wander(HeroPtr h) TimeCheck tc("looking for wander destination"); - while(h->movementPointsRemaining()) + for(int k = 0; k < 10 && h->movementPointsRemaining(); k++) { validateVisitableObjs(); ah->updatePaths(getMyHeroes()); From 1eb58bcc32d6cb98b0c9e21ad0d5adc2685c0388 Mon Sep 17 00:00:00 2001 From: Andrii Danylchenko <danilchenko.andrij@gmail.com> Date: Sun, 6 Aug 2023 08:57:14 +0300 Subject: [PATCH 17/20] NKAI: fix potential concurrency and town treat calculation --- .../Analyzers/DangerHitMapAnalyzer.cpp | 62 +++++++++++-------- .../Analyzers/DangerHitMapAnalyzer.h | 15 ++++- 2 files changed, 48 insertions(+), 29 deletions(-) diff --git a/AI/Nullkiller/Analyzers/DangerHitMapAnalyzer.cpp b/AI/Nullkiller/Analyzers/DangerHitMapAnalyzer.cpp index efc4bde8e..3651e567f 100644 --- a/AI/Nullkiller/Analyzers/DangerHitMapAnalyzer.cpp +++ b/AI/Nullkiller/Analyzers/DangerHitMapAnalyzer.cpp @@ -53,6 +53,13 @@ void DangerHitMapAnalyzer::updateHitMap() } } + auto ourTowns = cb->getTownsInfo(); + + for(auto town : ourTowns) + { + townTreats[town->id]; // insert empty list + } + foreach_tile_pos([&](const int3 & pos){ hitMap[pos.x][pos.y][pos.z].reset(); }); @@ -95,33 +102,33 @@ void DangerHitMapAnalyzer::updateHitMap() node.fastestDanger = newTreat; } - if(newTreat.turn == 0) + auto objects = cb->getVisitableObjs(pos, false); + + for(auto obj : objects) { - auto objects = cb->getVisitableObjs(pos, false); - - for(auto obj : objects) + if(obj->ID == Obj::TOWN && obj->getOwner() == ai->playerID) { - if(cb->getPlayerRelations(obj->tempOwner, ai->playerID) != PlayerRelations::ENEMIES) - enemyHeroAccessibleObjects[path.targetHero].insert(obj); + auto & treats = townTreats[obj->id]; + auto treat = std::find_if(treats.begin(), treats.end(), [&](const HitMapInfo & i) -> bool + { + return i.hero.hid == path.targetHero->id; + }); - if(obj->ID == Obj::TOWN && obj->getOwner() == ai->playerID) + if(treat == treats.end()) { - auto & treats = townTreats[obj->id]; - auto treat = std::find_if(treats.begin(), treats.end(), [&](const HitMapInfo & i) -> bool - { - return i.hero.hid == path.targetHero->id; - }); + treats.emplace_back(); + treat = std::prev(treats.end(), 1); + } - if(treat == treats.end()) - { - treats.emplace_back(); - treat = std::prev(treats.end(), 1); - } + if(newTreat.value() > treat->value()) + { + *treat = newTreat; + } - if(newTreat.value() > treat->value()) - { - *treat = newTreat; - } + if(newTreat.turn == 0) + { + if(cb->getPlayerRelations(obj->tempOwner, ai->playerID) != PlayerRelations::ENEMIES) + enemyHeroAccessibleObjects.emplace_back(path.targetHero, obj); } } } @@ -274,16 +281,17 @@ const HitMapNode & DangerHitMapAnalyzer::getTileTreat(const int3 & tile) const const std::set<const CGObjectInstance *> empty = {}; -const std::set<const CGObjectInstance *> & DangerHitMapAnalyzer::getOneTurnAccessibleObjects(const CGHeroInstance * enemy) const +std::set<const CGObjectInstance *> DangerHitMapAnalyzer::getOneTurnAccessibleObjects(const CGHeroInstance * enemy) const { - auto result = enemyHeroAccessibleObjects.find(enemy); - - if(result == enemyHeroAccessibleObjects.end()) + std::set<const CGObjectInstance *> result; + + for(auto & obj : enemyHeroAccessibleObjects) { - return empty; + if(obj.hero == enemy) + result.insert(obj.obj); } - return result->second; + return result; } void DangerHitMapAnalyzer::reset() diff --git a/AI/Nullkiller/Analyzers/DangerHitMapAnalyzer.h b/AI/Nullkiller/Analyzers/DangerHitMapAnalyzer.h index 79c56c2c4..614312649 100644 --- a/AI/Nullkiller/Analyzers/DangerHitMapAnalyzer.h +++ b/AI/Nullkiller/Analyzers/DangerHitMapAnalyzer.h @@ -55,11 +55,22 @@ struct HitMapNode } }; +struct EnemyHeroAccessibleObject +{ + const CGHeroInstance * hero; + const CGObjectInstance * obj; + + EnemyHeroAccessibleObject(const CGHeroInstance * hero, const CGObjectInstance * obj) + :hero(hero), obj(obj) + { + } +}; + class DangerHitMapAnalyzer { private: boost::multi_array<HitMapNode, 3> hitMap; - std::map<const CGHeroInstance *, std::set<const CGObjectInstance *>> enemyHeroAccessibleObjects; + tbb::concurrent_vector<EnemyHeroAccessibleObject> enemyHeroAccessibleObjects; bool hitMapUpToDate = false; bool tileOwnersUpToDate = false; const Nullkiller * ai; @@ -73,7 +84,7 @@ public: uint64_t enemyCanKillOurHeroesAlongThePath(const AIPath & path) const; const HitMapNode & getObjectTreat(const CGObjectInstance * obj) const; const HitMapNode & getTileTreat(const int3 & tile) const; - const std::set<const CGObjectInstance *> & getOneTurnAccessibleObjects(const CGHeroInstance * enemy) const; + std::set<const CGObjectInstance *> getOneTurnAccessibleObjects(const CGHeroInstance * enemy) const; void reset(); void resetTileOwners() { tileOwnersUpToDate = false; } PlayerColor getTileOwner(const int3 & tile) const; From ed066cb7ba2162a872a5c8031d0c698fd06e8fd7 Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Tue, 8 Aug 2023 00:27:03 +0300 Subject: [PATCH 18/20] Fixed possible multithreading races in sound/music callbacks --- client/CMusicHandler.cpp | 89 ++++++++++++++++++++++------------------ client/CMusicHandler.h | 13 ++++-- 2 files changed, 60 insertions(+), 42 deletions(-) diff --git a/client/CMusicHandler.cpp b/client/CMusicHandler.cpp index a1a31370b..521d47eb1 100644 --- a/client/CMusicHandler.cpp +++ b/client/CMusicHandler.cpp @@ -14,6 +14,7 @@ #include "CMusicHandler.h" #include "CGameInfo.h" #include "renderSDL/SDLRWwrapper.h" +#include "gui/CGuiHandler.h" #include "../lib/JsonNode.h" #include "../lib/GameConstants.h" @@ -185,9 +186,9 @@ int CSoundHandler::playSound(std::string sound, int repeats, bool cache) Mix_FreeChunk(chunk); } else if (cache) - callbacks[channel]; + initCallback(channel); else - callbacks[channel] = [chunk](){ Mix_FreeChunk(chunk);}; + initCallback(channel, [chunk](){ Mix_FreeChunk(chunk);}); } else channel = -1; @@ -237,28 +238,50 @@ void CSoundHandler::setChannelVolume(int channel, ui32 percent) void CSoundHandler::setCallback(int channel, std::function<void()> function) { - std::map<int, std::function<void()> >::iterator iter; - iter = callbacks.find(channel); + boost::unique_lock lockGuard(mutexCallbacks); + + auto iter = callbacks.find(channel); //channel not found. It may have finished so fire callback now if(iter == callbacks.end()) function(); else - iter->second = function; + iter->second.push_back(function); } void CSoundHandler::soundFinishedCallback(int channel) { - std::map<int, std::function<void()> >::iterator iter; - iter = callbacks.find(channel); - if (iter == callbacks.end()) + boost::unique_lock lockGuard(mutexCallbacks); + + if (callbacks.count(channel) == 0) return; - auto callback = std::move(iter->second); - callbacks.erase(iter); + // store callbacks from container locally - SDL might reuse this channel for another sound + // but do actualy execution in separate thread, to avoid potential deadlocks in case if callback requires locks of its own + auto callback = callbacks.at(channel); + callbacks.erase(channel); - if (callback) - callback(); + if (!callback.empty()) + { + GH.dispatchMainThread([callback](){ + for (auto entry : callback) + entry(); + }); + } +} + +void CSoundHandler::initCallback(int channel) +{ + boost::unique_lock lockGuard(mutexCallbacks); + assert(callbacks.count(channel) == 0); + callbacks[channel] = {}; +} + +void CSoundHandler::initCallback(int channel, const std::function<void()> & function) +{ + boost::unique_lock lockGuard(mutexCallbacks); + assert(callbacks.count(channel) == 0); + callbacks[channel].push_back(function); } int CSoundHandler::ambientGetRange() const @@ -471,44 +494,32 @@ void CMusicHandler::setVolume(ui32 percent) void CMusicHandler::musicFinishedCallback() { - // boost::mutex::scoped_lock guard(mutex); - // FIXME: WORKAROUND FOR A POTENTIAL DEADLOCK + // call music restart in separate thread to avoid deadlock in some cases // It is possible for: // 1) SDL thread to call this method on end of playback // 2) VCMI code to call queueNext() method to queue new file // this leads to: // 1) SDL thread waiting to acquire music lock in this method (while keeping internal SDL mutex locked) // 2) VCMI thread waiting to acquire internal SDL mutex (while keeping music mutex locked) - // Because of that (and lack of clear way to fix that) - // We will try to acquire lock here and if failed - do nothing - // This may break music playback till next song is enqued but won't deadlock the game - if (!mutex.try_lock()) + GH.dispatchMainThread([this]() { - logGlobal->error("Failed to acquire mutex! Unable to restart music!"); - return; - } - - if (current.get() != nullptr) - { - // if music is looped, play it again - if (current->play()) + boost::unique_lock lockGuard(mutex); + if (current.get() != nullptr) { - mutex.unlock(); - return; + // if music is looped, play it again + if (current->play()) + return; + else + current.reset(); } - else - { - current.reset(); - } - } - if (current.get() == nullptr && next.get() != nullptr) - { - current.reset(next.release()); - current->play(); - } - mutex.unlock(); + if (current.get() == nullptr && next.get() != nullptr) + { + current.reset(next.release()); + current->play(); + } + }); } MusicEntry::MusicEntry(CMusicHandler *owner, std::string setName, std::string musicURI, bool looped, bool fromStart): diff --git a/client/CMusicHandler.h b/client/CMusicHandler.h index 3594cf37d..a0180cd74 100644 --- a/client/CMusicHandler.h +++ b/client/CMusicHandler.h @@ -45,9 +45,13 @@ private: Mix_Chunk *GetSoundChunk(std::string &sound, bool cache); - //have entry for every currently active channel - //std::function will be nullptr if callback was not set - std::map<int, std::function<void()> > callbacks; + /// have entry for every currently active channel + /// vector will be empty if callback was not set + std::map<int, std::vector<std::function<void()>> > callbacks; + + /// Protects access to callbacks member to avoid data races: + /// SDL calls sound finished callbacks from audio thread + boost::mutex mutexCallbacks; int ambientDistToVolume(int distance) const; void ambientStopSound(std::string soundId); @@ -58,6 +62,9 @@ private: std::map<std::string, int> ambientChannels; std::map<int, int> channelVolumes; + void initCallback(int channel, const std::function<void()> & function); + void initCallback(int channel); + public: CSoundHandler(); From d947c14495cb8dd8caa255f1df43b9879dc6005d Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Tue, 8 Aug 2023 12:50:16 +0300 Subject: [PATCH 19/20] Few more H3 data validation checks --- client/CMT.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/client/CMT.cpp b/client/CMT.cpp index b744aee4c..7e4feb25f 100644 --- a/client/CMT.cpp +++ b/client/CMT.cpp @@ -256,16 +256,16 @@ int main(int argc, char * argv[]) logGlobal->debug("settings = %s", settings.toJsonNode().toJson()); // Some basic data validation to produce better error messages in cases of incorrect install - auto testFile = [](std::string filename, std::string message) -> bool + auto testFile = [](std::string filename, std::string message) { - if (CResourceHandler::get()->existsResource(ResourceID(filename))) - return true; - - handleFatalError(message, false); + if (!CResourceHandler::get()->existsResource(ResourceID(filename))) + handleFatalError(message, false); }; testFile("DATA/HELP.TXT", "VCMI requires Heroes III: Shadow of Death or Heroes III: Complete data files to run!"); testFile("MODS/VCMI/MOD.JSON", "VCMI installation is corrupted! Built-in mod was not found!"); + testFile("DATA/PLAYERS.PAL", "Heroes III data files are missing or corruped! Please reinstall them."); + testFile("SPRITES/DEFAULT.DEF", "Heroes III data files are missing or corruped! Please reinstall them."); testFile("DATA/TENTCOLR.TXT", "Heroes III: Restoration of Erathia (including HD Edition) data files are not supported!"); srand ( (unsigned int)time(nullptr) ); From 2e7e8bb52bc6a1cd59da84dc894d3623f6dcaa58 Mon Sep 17 00:00:00 2001 From: Ivan Savenko <saven.ivan@gmail.com> Date: Tue, 8 Aug 2023 12:50:39 +0300 Subject: [PATCH 20/20] Better error message on missing object constructor --- .../CObjectClassesHandler.cpp | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/lib/mapObjectConstructors/CObjectClassesHandler.cpp b/lib/mapObjectConstructors/CObjectClassesHandler.cpp index 22265c0cb..99d5aa82c 100644 --- a/lib/mapObjectConstructors/CObjectClassesHandler.cpp +++ b/lib/mapObjectConstructors/CObjectClassesHandler.cpp @@ -316,11 +316,21 @@ std::vector<bool> CObjectClassesHandler::getDefaultAllowed() const TObjectTypeHandler CObjectClassesHandler::getHandlerFor(si32 type, si32 subtype) const { - assert(type < objects.size()); - assert(objects[type]); - assert(subtype < objects[type]->objects.size()); + try + { + auto result = objects.at(type)->objects.at(subtype); - return objects.at(type)->objects.at(subtype); + if (result != nullptr) + return result; + } + catch (std::out_of_range & e) + { + // Leave catch block silently + } + + std::string errorString = "Failed to find object of type " + std::to_string(type) + "::" + std::to_string(subtype); + logGlobal->error(errorString); + throw std::runtime_error(errorString); } TObjectTypeHandler CObjectClassesHandler::getHandlerFor(const std::string & scope, const std::string & type, const std::string & subtype) const