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); 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 } } + if (exported == null) + { + publishProgress("Failed to copy file " + child.getName()); + return false; + } + try( final OutputStream targetStream = owner.getContentResolver() .openOutputStream(exported.getUri()); 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..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 @@ -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 || dir.listFiles() == null) + { + Log.e("Broken path given to fileutil::clearDirectory"); + return false; + } + for (final File f : dir.listFiles()) { if (f.isDirectory() && !clearDirectory(f)) diff --git a/client/CMT.cpp b/client/CMT.cpp index 3f4fe83b4..7e4feb25f 100644 --- a/client/CMT.cpp +++ b/client/CMT.cpp @@ -38,6 +38,7 @@ #include #include +#include #ifdef VCMI_ANDROID #include "../lib/CAndroidVMHelper.h" @@ -255,24 +256,17 @@ 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; - - logGlobal->error("Error: %s was not found!", message); - return false; + if (!CResourceHandler::get()->existsResource(ResourceID(filename))) + 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/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) ); @@ -510,3 +504,18 @@ void handleQuit(bool ask) quitApplication(); } } + +void handleFatalError(const std::string & message, bool terminate) +{ + logGlobal->error("FATAL ERROR ENCOUTERED, VCMI WILL NOW TERMINATE"); + logGlobal->error("Reason: %s", message); + + std::string messageToShow = "Fatal 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 887b704c9..28d877875 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, bool terminate); 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 function) { - std::map >::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 >::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 & 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 > callbacks; + /// have entry for every currently active channel + /// vector will be empty if callback was not set + std::map> > 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 ambientChannels; std::map channelVolumes; + void initCallback(int channel, const std::function & function); + void initCallback(int channel); + public: CSoundHandler(); 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 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 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/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); 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 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 } 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; diff --git a/client/renderSDL/SDL_Extensions.cpp b/client/renderSDL/SDL_Extensions.cpp index b362e137f..b4d7f1249 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, true); + } + if (mod->format->palette) { assert(ret->format->palette); diff --git a/client/renderSDL/ScreenHandler.cpp b/client/renderSDL/ScreenHandler.cpp index 8de235d47..623289074 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, true); + } //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); 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); 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(); 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!"); } } 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 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 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 battleMadeAction(false); +boost::recursive_mutex battleActionMutex; CondSh battleResult(nullptr); template 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 }