Merge pull request #2505 from IvanSavenko/hotfix_3

Hotfix 3

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

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;

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
 			{

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

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

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

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;

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

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
+
+		if (current.get() == nullptr && next.get() != nullptr)
 		{
-			current.reset();
+			current.reset(next.release());
+			current->play();
 		}
-	}
-
-	if (current.get() == nullptr && next.get() != nullptr)
-	{
-		current.reset(next.release());
-		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
-	//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();
 

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);
+
+		if (result != nullptr)
+			return result;
+	}
+	catch (std::out_of_range & e)
+	{
+		// Leave catch block silently
+	}
 
-	return objects.at(type)->objects.at(subtype);
+	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
 						}