Merge pull request #3534 from IvanSavenko/crashfixes

Crashfixes

AI/StupidAI/StupidAI.cpp

@@ -16,8 +16,6 @@
 #include "../../lib/battle/BattleAction.h"
 #include "../../lib/battle/BattleInfo.h"
 
-static std::shared_ptr<CBattleCallback> cbc;
-
 CStupidAI::CStupidAI()
 	: side(-1)
 	, wasWaitingForRealize(false)
@@ -41,7 +39,7 @@ void CStupidAI::initBattleInterface(std::shared_ptr<Environment> ENV, std::share
 {
 	print("init called, saving ptr to IBattleCallback");
 	env = ENV;
-	cbc = cb = CB;
+	cb = CB;
 
 	wasWaitingForRealize = CB->waitTillRealize;
 	wasUnlockingGs = CB->unlockGsWhenWaiting;
@@ -72,11 +70,11 @@ public:
 	std::vector<BattleHex> attackFrom; //for melee fight
 	EnemyInfo(const CStack * _s) : s(_s), adi(0), adr(0)
 	{}
-	void calcDmg(const BattleID & battleID, const CStack * ourStack)
+	void calcDmg(std::shared_ptr<CBattleCallback> cb, const BattleID & battleID, const CStack * ourStack)
 	{
 		// FIXME: provide distance info for Jousting bonus
 		DamageEstimation retal;
-		DamageEstimation dmg = cbc->getBattle(battleID)->battleEstimateDamage(ourStack, s, 0, &retal);
+		DamageEstimation dmg = cb->getBattle(battleID)->battleEstimateDamage(ourStack, s, 0, &retal);
 		adi = static_cast<int>((dmg.damage.min + dmg.damage.max) / 2);
 		adr = static_cast<int>((retal.damage.min + retal.damage.max) / 2);
 	}
@@ -92,14 +90,14 @@ bool isMoreProfitable(const EnemyInfo &ei1, const EnemyInfo& ei2)
 	return (ei1.adi-ei1.adr) < (ei2.adi - ei2.adr);
 }
 
-static bool willSecondHexBlockMoreEnemyShooters(const BattleID & battleID, const BattleHex &h1, const BattleHex &h2)
+static bool willSecondHexBlockMoreEnemyShooters(std::shared_ptr<CBattleCallback> cb, const BattleID & battleID, const BattleHex &h1, const BattleHex &h2)
 {
 	int shooters[2] = {0}; //count of shooters on hexes
 
 	for(int i = 0; i < 2; i++)
 	{
 		for (auto & neighbour : (i ? h2 : h1).neighbouringTiles())
-			if(const auto * s = cbc->getBattle(battleID)->battleGetUnitByPos(neighbour))
+			if(const auto * s = cb->getBattle(battleID)->battleGetUnitByPos(neighbour))
 				if(s->isShooter())
 					shooters[i]++;
 	}
@@ -169,10 +167,10 @@ void CStupidAI::activeStack(const BattleID & battleID, const CStack * stack)
 	}
 
 	for ( auto & enemy : enemiesReachable )
-		enemy.calcDmg(battleID, stack);
+		enemy.calcDmg(cb, battleID, stack);
 
 	for ( auto & enemy : enemiesShootable )
-		enemy.calcDmg(battleID, stack);
+		enemy.calcDmg(cb, battleID, stack);
 
 	if(enemiesShootable.size())
 	{
@@ -183,7 +181,7 @@ void CStupidAI::activeStack(const BattleID & battleID, const CStack * stack)
 	else if(enemiesReachable.size())
 	{
 		const EnemyInfo &ei= *std::max_element(enemiesReachable.begin(), enemiesReachable.end(), &isMoreProfitable);
-		BattleHex targetHex = *std::max_element(ei.attackFrom.begin(), ei.attackFrom.end(), [&](auto a, auto b) { return willSecondHexBlockMoreEnemyShooters(battleID, a, b);});
+		BattleHex targetHex = *std::max_element(ei.attackFrom.begin(), ei.attackFrom.end(), [&](auto a, auto b) { return willSecondHexBlockMoreEnemyShooters(cb, battleID, a, b);});
 
 		cb->battleMakeUnitAction(battleID, BattleAction::makeMeleeAttack(stack, ei.s->getPosition(), targetHex));
 		return;

android/vcmi-app/build.gradle

@@ -10,7 +10,7 @@ android {
 		applicationId "is.xyz.vcmi"
 		minSdk 19
 		targetSdk 33
-		versionCode 1440
+		versionCode 1441
 		versionName "1.4.4"
 		setProperty("archivesBaseName", "vcmi")
 	}

client/battle/BattleActionsController.cpp

@@ -750,7 +750,7 @@ void BattleActionsController::actionRealize(PossiblePlayerBattleAction action, B
 
 			if (!spellcastingModeActive())
 			{
-				if (action.spell().toSpell())
+				if (action.spell().hasValue())
 				{
 					owner.giveCommand(EActionType::MONSTER_SPELL, targetHex, action.spell());
 				}
@@ -887,17 +887,17 @@ void BattleActionsController::tryActivateStackSpellcasting(const CStack *casterS
 	{
 		// faerie dragon can cast only one, randomly selected spell until their next move
 		//TODO: faerie dragon type spell should be selected by server
-		const auto * spellToCast = owner.getBattle()->getRandomCastedSpell(CRandomGenerator::getDefault(), casterStack).toSpell();
+		const auto spellToCast = owner.getBattle()->getRandomCastedSpell(CRandomGenerator::getDefault(), casterStack);
 
-		if (spellToCast)
-			creatureSpells.push_back(spellToCast);
+		if (spellToCast.hasValue())
+			creatureSpells.push_back(spellToCast.toSpell());
 	}
 
 	TConstBonusListPtr bl = casterStack->getBonuses(Selector::type()(BonusType::SPELLCASTER));
 
 	for(const auto & bonus : *bl)
 	{
-		if (bonus->additionalInfo[0] <= 0)
+		if (bonus->additionalInfo[0] <= 0 && bonus->subtype.as<SpellID>().hasValue())
 			creatureSpells.push_back(bonus->subtype.as<SpellID>().toSpell());
 	}
 }

client/battle/BattleInterface.cpp

@@ -352,13 +352,13 @@ void BattleInterface::spellCast(const BattleSpellCast * sc)
 	CCS->curh->set(Cursor::Combat::BLOCKED);
 
 	const SpellID spellID = sc->spellID;
+
+	if(!spellID.hasValue())
+		return;
+
 	const CSpell * spell = spellID.toSpell();
 	auto targetedTile = sc->tile;
 
-	assert(spell);
-	if(!spell)
-		return;
-
 	const AudioPath & castSoundPath = spell->getCastSound();
 
 	if (!castSoundPath.empty())

client/renderSDL/SDLImage.cpp

@@ -205,6 +205,8 @@ void SDLImage::exportBitmap(const boost::filesystem::path& path) const
 
 void SDLImage::playerColored(PlayerColor player)
 {
+	if (!surf)
+		return;
 	graphics->blueToPlayersAdv(surf, player);
 }
 

lib/JsonRandom.cpp

@@ -82,7 +82,7 @@ namespace JsonRandom
 	IdentifierType decodeKey(const std::string & modScope, const std::string & value, const Variables & variables)
 	{
 		if (value.empty() || value[0] != '@')
-			return IdentifierType(*VLC->identifiers()->getIdentifier(modScope, IdentifierType::entityType(), value));
+			return IdentifierType(VLC->identifiers()->getIdentifier(modScope, IdentifierType::entityType(), value).value_or(-1));
 		else
 			return loadVariable(IdentifierType::entityType(), value, variables, IdentifierType::NONE);
 	}
@@ -91,7 +91,7 @@ namespace JsonRandom
 	IdentifierType decodeKey(const JsonNode & value, const Variables & variables)
 	{
 		if (value.String().empty() || value.String()[0] != '@')
-			return IdentifierType(*VLC->identifiers()->getIdentifier(IdentifierType::entityType(), value));
+			return IdentifierType(VLC->identifiers()->getIdentifier(IdentifierType::entityType(), value).value_or(-1));
 		else
 			return loadVariable(IdentifierType::entityType(), value.String(), variables, IdentifierType::NONE);
 	}

lib/battle/CBattleInfoCallback.cpp

@@ -867,9 +867,10 @@ bool CBattleInfoCallback::handleObstacleTriggersForUnit(SpellCastEnvironment & s
 			auto shouldReveal = !spellObstacle->hidden || !battleIsObstacleVisibleForSide(*obstacle, (BattlePerspective::BattlePerspective)side);
 			const auto * hero = battleGetFightingHero(spellObstacle->casterSide);
 			auto caster = spells::ObstacleCasterProxy(getBattle()->getSidePlayer(spellObstacle->casterSide), hero, *spellObstacle);
-			const auto * sp = obstacle->getTrigger().toSpell();
-			if(obstacle->triggersEffects() && sp)
+
+			if(obstacle->triggersEffects() && obstacle->getTrigger().hasValue())
 			{
+				const auto * sp = obstacle->getTrigger().toSpell();
 				auto cast = spells::BattleCast(this, &caster, spells::Mode::PASSIVE, sp);
 				spells::detail::ProblemImpl ignored;
 				auto target = spells::Target(1, spells::Destination(&unit));

lib/constants/IdentifierBase.h

@@ -52,6 +52,11 @@ public:
 		num = value;
 	}
 
+	constexpr bool hasValue() const
+	{
+		return num >= 0;
+	}
+
 	struct hash
 	{
 		size_t operator()(const IdentifierBase & id) const

lib/mapObjectConstructors/AObjectTypeHandler.cpp

@@ -21,17 +21,7 @@
 VCMI_LIB_NAMESPACE_BEGIN
 
 AObjectTypeHandler::AObjectTypeHandler() = default;
-
-AObjectTypeHandler::~AObjectTypeHandler()
-{
-	// FIXME: currently on Android there is a weird crash in destructor of 'base' member
-	// this code attempts to localize and fix this crash
-	if (base)
-	{
-		base->clear();
-		base.reset();
-	}
-}
+AObjectTypeHandler::~AObjectTypeHandler() = default;
 
 std::string AObjectTypeHandler::getJsonKey() const
 {
@@ -89,12 +79,12 @@ void AObjectTypeHandler::init(const JsonNode & input)
 		if (base)
 			JsonUtils::inherit(entry.second, *base);
 
-		auto * tmpl = new ObjectTemplate;
+		auto tmpl = std::make_shared<ObjectTemplate>();
 		tmpl->id = Obj(type);
 		tmpl->subid = subtype;
 		tmpl->stringID = entry.first; // FIXME: create "fullID" - type.object.template?
 		tmpl->readJson(entry.second);
-		templates.push_back(std::shared_ptr<const ObjectTemplate>(tmpl));
+		templates.push_back(tmpl);
 	}
 
 	for(const JsonNode & node : input["sounds"]["ambient"].Vector())
@@ -188,12 +178,13 @@ void AObjectTypeHandler::addTemplate(JsonNode config)
 	config.setType(JsonNode::JsonType::DATA_STRUCT); // ensure that input is not null
 	if (base)
 		JsonUtils::inherit(config, *base);
-	auto * tmpl = new ObjectTemplate;
+
+	auto tmpl = std::make_shared<ObjectTemplate>();
 	tmpl->id = Obj(type);
 	tmpl->subid = subtype;
 	tmpl->stringID.clear(); // TODO?
 	tmpl->readJson(config);
-	templates.emplace_back(tmpl);
+	templates.push_back(tmpl);
 }
 
 std::vector<std::shared_ptr<const ObjectTemplate>> AObjectTypeHandler::getTemplates() const

lib/mapObjectConstructors/CObjectClassesHandler.cpp

@@ -109,13 +109,13 @@ std::vector<JsonNode> CObjectClassesHandler::loadLegacyData()
 
 	for (size_t i = 0; i < totalNumber; i++)
 	{
-		auto * tmpl = new ObjectTemplate;
+		auto tmpl = std::make_shared<ObjectTemplate>();
 
 		tmpl->readTxt(parser);
 		parser.endLine();
 
 		std::pair key(tmpl->id, tmpl->subid);
-		legacyTemplates.insert(std::make_pair(key, std::shared_ptr<const ObjectTemplate>(tmpl)));
+		legacyTemplates.insert(std::make_pair(key, tmpl));
 	}
 
 	objects.resize(256);

lib/mapObjectConstructors/DwellingInstanceConstructor.cpp

@@ -35,17 +35,17 @@ void DwellingInstanceConstructor::initTypeData(const JsonNode & input)
 	const auto totalLevels = levels.size();
 
 	availableCreatures.resize(totalLevels);
-	for(auto currentLevel = 0; currentLevel < totalLevels; currentLevel++)
+	for(int currentLevel = 0; currentLevel < totalLevels; currentLevel++)
 	{
 		const JsonVector & creaturesOnLevel = levels[currentLevel].Vector();
 		const auto creaturesNumber = creaturesOnLevel.size();
 		availableCreatures[currentLevel].resize(creaturesNumber);
 
-		for(auto currentCreature = 0; currentCreature < creaturesNumber; currentCreature++)
+		for(int currentCreature = 0; currentCreature < creaturesNumber; currentCreature++)
 		{
-			VLC->identifiers()->requestIdentifier("creature", creaturesOnLevel[currentCreature], [=] (si32 index)
+			VLC->identifiers()->requestIdentifier("creature", creaturesOnLevel[currentCreature], [this, currentLevel, currentCreature] (si32 index)
 			{
-				availableCreatures[currentLevel][currentCreature] = VLC->creh->objects[index];
+				availableCreatures.at(currentLevel).at(currentCreature) = VLC->creh->objects[index];
 			});
 		}
 		assert(!availableCreatures[currentLevel].empty());

lib/mapping/MapFormatJson.cpp

@@ -1074,14 +1074,14 @@ void CMapLoaderJson::MapObjectLoader::construct()
 
 	auto handler = VLC->objtypeh->getHandlerFor( ModScope::scopeMap(), typeName, subtypeName);
 
-	auto * appearance = new ObjectTemplate;
+	auto appearance = std::make_shared<ObjectTemplate>();
 
 	appearance->id = Obj(handler->getIndex());
 	appearance->subid = handler->getSubIndex();
 	appearance->readJson(configuration["template"], false);
 
 	// Will be destroyed soon and replaced with shared template
-	instance = handler->create(std::shared_ptr<const ObjectTemplate>(appearance));
+	instance = handler->create(appearance);
 
 	instance->id = ObjectInstanceID(static_cast<si32>(owner->map->objects.size()));
 	instance->instanceName = jsonKey;

server/CGameHandler.cpp

@@ -3955,14 +3955,14 @@ bool CGameHandler::moveStack(const StackLocation &src, const StackLocation &dst,
 
 void CGameHandler::castSpell(const spells::Caster * caster, SpellID spellID, const int3 &pos)
 {
-	const CSpell * s = spellID.toSpell();
-	if(!s)
+	if (!spellID.hasValue())
 		return;
 
 	AdventureSpellCastParameters p;
 	p.caster = caster;
 	p.pos = pos;
 
+	const CSpell * s = spellID.toSpell();
 	s->adventureCast(spellEnv, p);
 }
 

server/NetPacksServer.cpp

@@ -327,9 +327,9 @@ void ApplyGhNetPackVisitor::visitCastAdvSpell(CastAdvSpell & pack)
 {
 	gh.throwIfWrongOwner(&pack, pack.hid);
 
-	const CSpell * s = pack.sid.toSpell();
-	if(!s)
+	if (!pack.sid.hasValue())
 		gh.throwNotAllowedAction(&pack);
+
 	const CGHeroInstance * h = gh.getHero(pack.hid);
 	if(!h)
 		gh.throwNotAllowedAction(&pack);
@@ -338,6 +338,7 @@ void ApplyGhNetPackVisitor::visitCastAdvSpell(CastAdvSpell & pack)
 	p.caster = h;
 	p.pos = pack.pos;
 
+	const CSpell * s = pack.sid.toSpell();
 	result = s->adventureCast(gh.spellEnv, p);
 }
 

server/TurnTimerHandler.cpp

@@ -88,21 +88,21 @@ void TurnTimerHandler::onPlayerGetTurn(PlayerColor player)
 void TurnTimerHandler::update(int waitTime)
 {
 	std::lock_guard<std::recursive_mutex> guard(mx);
-	if(const auto * gs = gameHandler.gameState())
-	{
-		for(PlayerColor player(0); player < PlayerColor::PLAYER_LIMIT; ++player)
-			if(gs->isPlayerMakingTurn(player))
-				onPlayerMakingTurn(player, waitTime);
-		
-		// create copy for iterations - battle might end during onBattleLoop call
-		std::vector<BattleID> ongoingBattles;
+	if(!gameHandler.getStartInfo()->turnTimerInfo.isEnabled())
+		return;
 
-		for (auto & battle : gs->currentBattles)
-			ongoingBattles.push_back(battle->battleID);
+	for(PlayerColor player(0); player < PlayerColor::PLAYER_LIMIT; ++player)
+		if(gameHandler.gameState()->isPlayerMakingTurn(player))
+			onPlayerMakingTurn(player, waitTime);
 
-		for (auto & battleID : ongoingBattles)
-			onBattleLoop(battleID, waitTime);
-	}
+	// create copy for iterations - battle might end during onBattleLoop call
+	std::vector<BattleID> ongoingBattles;
+
+	for (auto & battle : gameHandler.gameState()->currentBattles)
+		ongoingBattles.push_back(battle->battleID);
+
+	for (auto & battleID : ongoingBattles)
+		onBattleLoop(battleID, waitTime);
 }
 
 bool TurnTimerHandler::timerCountDown(int & timer, int initialTimer, PlayerColor player, int waitTime)

server/battles/BattleActionProcessor.cpp

@@ -102,13 +102,13 @@ bool BattleActionProcessor::doHeroSpellAction(const CBattleInfoCallback & battle
 		return false;
 	}
 
-	const CSpell * s = ba.spell.toSpell();
-	if (!s)
+	if (!ba.spell.hasValue())
 	{
 		logGlobal->error("Wrong spell id (%d)!", ba.spell.getNum());
 		return false;
 	}
 
+	const CSpell * s = ba.spell.toSpell();
 	spells::BattleCast parameters(&battle, h, spells::Mode::HERO, s);
 
 	spells::detail::ProblemImpl problem;