Merged master into beta

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);

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());

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))

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"
@@ -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)))
+		if (!CResourceHandler::get()->existsResource(ResourceID(filename)))
-			return true;
+			handleFatalError(message, false);
-
-		logGlobal->error("Error: %s was not found!", message);
-		return false;
 	};
 
-	if (!testFile("DATA/HELP.TXT", "Heroes III data") ||
+	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 data"))
+	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.");
-		exit(1); // These are unrecoverable errors
+	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!");
-
-	// 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
 
 	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);
+}

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);

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;
+	boost::unique_lock lockGuard(mutexCallbacks);
-	iter = callbacks.find(channel);
+
+	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;
+	boost::unique_lock lockGuard(mutexCallbacks);
-	iter = callbacks.find(channel);
+
-	if (iter == callbacks.end())
+	if (callbacks.count(channel) == 0)
 		return;
 
-	auto callback = std::move(iter->second);
+	// store callbacks from container locally - SDL might reuse this channel for another sound
-	callbacks.erase(iter);
+	// 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)
+	if (!callback.empty())
-		callback();
+	{
+		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);
+	// call music restart in separate thread to avoid deadlock in some cases
-	// FIXME: WORKAROUND FOR A POTENTIAL DEADLOCK
 	// 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!");
+		boost::unique_lock lockGuard(mutex);
-		return;
+		if (current.get() != nullptr)
-	}
-
-	if (current.get() != nullptr)
-	{
-		// if music is looped, play it again
-		if (current->play())
 		{
-			mutex.unlock();
+			// if music is looped, play it again
-			return;
+			if (current->play())
+				return;
+			else
+				current.reset();
 		}
-		else
-		{
-			current.reset();
-		}
-	}
 
-	if (current.get() == nullptr && next.get() != nullptr)
+		if (current.get() == nullptr && next.get() != nullptr)
-	{
+		{
-		current.reset(next.release());
+			current.reset(next.release());
-		current->play();
+			current->play();
-	}
+		}
-	mutex.unlock();
+	});
 }
 
 MusicEntry::MusicEntry(CMusicHandler *owner, std::string setName, std::string musicURI, bool looped, bool fromStart):

client/CMusicHandler.h

@@ -45,9 +45,13 @@ private:
 
 	Mix_Chunk *GetSoundChunk(std::string &sound, bool cache);
 
-	//have entry for every currently active channel
+	/// have entry for every currently active channel
-	//std::function will be nullptr if callback was not set
+	/// vector will be empty if callback was not set
-	std::map<int, std::function<void()> > callbacks;
+	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();
 

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, {});
 }

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();

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);
 

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
 }

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;

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);

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);

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);

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->error("Unexpected length of town buildings requirements: %d", node.Vector().size());
-				logMod->warn("Entry contains: ");
+				logMod->error("Entry contains: ");
-				logMod->warn(node.toJson());
+				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();

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!");
 	}
 }
 

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());
+	try
-	assert(objects[type]);
+	{
-	assert(subtype < objects[type]->objects.size());
+		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

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
 						}