Fix some issues detected by Sonar & code review, fix build

AI/Nullkiller/Goals/CompleteQuest.cpp

@@ -185,29 +185,6 @@ TGoalVec CompleteQuest::missionKeymaster(const Nullkiller * ai) const
 TGoalVec CompleteQuest::missionResources(const Nullkiller * ai) const
 {
 	TGoalVec solutions = tryCompleteQuest(ai);
-
-	/*auto heroes = cb->getHeroesInfo(); //TODO: choose best / free hero from among many possibilities?
-
-	if(heroes.size())
-	{
-		if(q.getQuest(cb)->checkQuest(heroes.front())) //it doesn't matter which hero it is
-		{
-			return solutions;// ai->ah->howToVisitObj(q.getObject(cb));
-		}
-		else
-		{
-			for(int i = 0; i < q.getQuest(cb)->m7resources.size(); ++i)
-			{
-				if(q.getQuest(cb)->m7resources[i])
-					solutions.push_back(sptr(CollectRes(static_cast<EGameResID>(i), q.getQuest(cb)->m7resources[i])));
-			}
-		}
-	}
-	else
-	{
-		solutions.push_back(sptr(Goals::RecruitHero())); //FIXME: checkQuest requires any hero belonging to player :(
-	}*/
-
 	return solutions;
 }
 

AI/VCAI/AIUtility.cpp

@@ -201,7 +201,10 @@ bool canBeEmbarkmentPoint(const TerrainTile * t, bool fromWater)
 bool isBlockedBorderGate(int3 tileToHit) //TODO: is that function needed? should be handled by pathfinder
 {
 	const auto * object = cb->getTopObj(tileToHit);
-	if( object && object->id != Obj::BORDER_GATE)
+	if(!object)
+		return false;
+
+	if(object->id != Obj::BORDER_GATE)
 		return false;
 
 	auto gate = dynamic_cast<const CGKeys *>(object);

AI/VCAI/VCAI.cpp

@@ -673,7 +673,7 @@ void VCAI::commanderGotLevel(const CCommanderInstance * commander, std::vector<u
 {
 	LOG_TRACE_PARAMS(logAi, "queryID '%i'", queryID);
 	NET_EVENT_HANDLER;
-	status.addQuery(queryID, boost::str(boost::format("Commander %s of %s got level %d") % commander->name % commander->getArmy()->nodeName() % (int)commander->level));
+	status.addQuery(queryID, boost::str(boost::format("Commander %s of %s got level %d") % commander->name % commander->getArmy()->nodeName() % static_cast<int>(commander->level)));
 	executeActionAsync("commanderGotLevel", [this, queryID](){ answerQuery(queryID, 0); });
 }
 

client/Client.cpp

@@ -227,7 +227,7 @@ void CClient::initPlayerEnvironments()
 		logNetwork->info("Preparing environment for player %s", color.toString());
 		playerEnvironments[color] = std::make_shared<CPlayerEnvironment>(color, this, std::make_shared<CCallback>(gamestate, color, this));
 		
-		if(!hasHumanPlayer && gameState()->players.at(color).isHuman())
+		if(color.isValidPlayer() && !hasHumanPlayer && gameState()->players.at(color).isHuman())
 			hasHumanPlayer = true;
 	}
 
@@ -247,7 +247,7 @@ void CClient::initPlayerEnvironments()
 
 void CClient::initPlayerInterfaces()
 {
-	for(auto & playerInfo : gameState()->getStartInfo()->playerInfos)
+	for(const auto & playerInfo : gameState()->getStartInfo()->playerInfos)
 	{
 		PlayerColor color = playerInfo.first;
 		if(!vstd::contains(GAME->server().getAllClientPlayers(GAME->server().logicConnection->connectionID), color))
@@ -259,7 +259,7 @@ void CClient::initPlayerInterfaces()
 			if(playerInfo.second.isControlledByAI() || settings["session"]["onlyai"].Bool())
 			{
 				bool alliedToHuman = false;
-				for(auto & allyInfo : gameState()->getStartInfo()->playerInfos)
+				for(const auto & allyInfo : gameState()->getStartInfo()->playerInfos)
 					if (gameState()->getPlayerTeam(allyInfo.first) == gameState()->getPlayerTeam(playerInfo.first) && allyInfo.second.isControlledByHuman())
 						alliedToHuman = true;
 

client/windows/CQuestLog.cpp

@@ -161,7 +161,7 @@ void CQuestLog::recreateLabelList()
 
 		MetaString text;
 		questPtr->getRolloverText(GAME->interface()->cb.get(), text, false);
-		if (quests[i].obj)
+		if (quests[i].obj.hasValue())
 		{
 			if (auto seersHut = dynamic_cast<const CGSeerHut *>(questObject))
 			{

lib/CArtHandler.cpp

@@ -1062,8 +1062,8 @@ void CArtifactSet::serializeJsonSlot(JsonSerializeFormat & handler, const Artifa
 }
 
 CArtifactFittingSet::CArtifactFittingSet(IGameCallback *cb, ArtBearer::ArtBearer bearer)
-	: GameCallbackHolder(cb)
-	, CArtifactSet(cb)
+	: CArtifactSet(cb)
+	, GameCallbackHolder(cb)
 	, bearer(bearer)
 {
 }

lib/CArtifactInstance.cpp

@@ -131,8 +131,8 @@ CArtifactInstance::CArtifactInstance(IGameCallback *cb, const CArtifact * art)
 }
 
 CArtifactInstance::CArtifactInstance(IGameCallback *cb)
-	: CCombinedArtifactInstance(cb)
-	, CBonusSystemNode(ARTIFACT_INSTANCE)
+	: CBonusSystemNode(ARTIFACT_INSTANCE)
+	, CCombinedArtifactInstance(cb)
 {
 }
 

lib/CArtifactInstance.h

@@ -72,7 +72,6 @@ public:
 class DLL_LINKAGE CArtifactInstance final
 	: public CBonusSystemNode, public CCombinedArtifactInstance, public CScrollArtifactInstance, public CGrowingArtifactInstance
 {
-protected:
 	ArtifactInstanceID id;
 	ArtifactID artTypeID;
 

lib/CCreatureSet.cpp

@@ -683,8 +683,8 @@ void CCreatureSet::serializeJson(JsonSerializeFormat & handler, const std::strin
 
 CStackInstance::CStackInstance(IGameCallback *cb, bool isHypothetic)
 	: CBonusSystemNode(isHypothetic)
-	, GameCallbackHolder(cb)
 	, CArtifactSet(cb)
+	, GameCallbackHolder(cb)
 	, nativeTerrain(this, Selector::type()(BonusType::TERRAIN_NATIVE))
 	, initiative(this, Selector::type()(BonusType::STACKS_SPEED))
 {

lib/CGameInfoCallback.cpp

@@ -759,7 +759,7 @@ int CPlayerSpecificInfoCallback::getHeroSerial(const CGHeroInstance * hero, bool
 
 	for (auto & possibleHero : heroes)
 	{
-		if (includeGarrisoned || !(possibleHero)->isGarrisoned())
+		if (includeGarrisoned || !possibleHero->isGarrisoned())
 			index++;
 
 		if (possibleHero == hero)

lib/CStack.h

@@ -108,22 +108,6 @@ public:
 		h & slot;
 		h & side;
 		h & initialPosition;
-
-//		const CArmedInstance * army = (base ? base->armyObj : nullptr);
-//		SlotID extSlot = (base ? base->armyObj->findStack(base) : SlotID());
-//
-//		if(h.saving)
-//		{
-//			h & army;
-//			h & extSlot;
-//		}
-//		else
-//		{
-//			h & army;
-//			h & extSlot;
-//
-//			postDeserialize(army, extSlot);
-//		}
 	}
 
 private:

lib/battle/BattleInfo.cpp

@@ -804,7 +804,7 @@ void BattleInfo::removeUnit(uint32_t id)
 			}
 
 			//cleanup remaining clone links if any
-			for(auto & s : stacks)
+			for(const auto & s : stacks)
 			{
 				if(s->cloneID == toRemoveId)
 					s->cloneID = -1;

lib/gameState/CGameState.cpp

@@ -1376,7 +1376,7 @@ bool CGameState::checkForVictory(const PlayerColor & player, const EventConditio
 		}
 		case EventCondition::DAYS_PASSED:
 		{
-			return (si32)day > condition.value;
+			return day > condition.value;
 		}
 		case EventCondition::IS_HUMAN:
 		{

lib/gameState/QuestInfo.h

@@ -38,9 +38,7 @@ struct DLL_LINKAGE QuestInfo //universal interface for human and AI
 
 	template <typename Handler> void serialize(Handler &h)
 	{
-		//h & quest;
 		h & obj;
-		//h & tile;
 	}
 };
 

lib/mapObjects/CGHeroInstance.cpp

@@ -1308,6 +1308,13 @@ const CGBoat * CGHeroInstance::getBoat() const
 	return nullptr;
 }
 
+CGBoat * CGHeroInstance::getBoat()
+{
+	if (boardedBoat.hasValue())
+		return dynamic_cast<CGBoat*>(cb->gameState()->getObjInstance(boardedBoat));
+	return nullptr;
+}
+
 void CGHeroInstance::setBoat(CGBoat* newBoat)
 {
 	assert(newBoat);
@@ -1334,9 +1341,10 @@ CBonusSystemNode * CGHeroInstance::whereShouldBeAttachedOnSiege(const bool isBat
 	if(!getVisitedTown())
 		return nullptr;
 
-	return isBattleOutsideTown ? (CBonusSystemNode *)(& getVisitedTown()->townAndVis)
-		: (CBonusSystemNode *)(getVisitedTown());
+	if (isBattleOutsideTown)
+		return const_cast<CTownAndVisitingHero *>(&getVisitedTown()->townAndVis);
 
+	return const_cast<CGTownInstance*>(getVisitedTown());
 }
 
 CBonusSystemNode * CGHeroInstance::whereShouldBeAttachedOnSiege(CGameState * gs)

lib/mapObjects/CGHeroInstance.h

@@ -60,7 +60,6 @@ class DLL_LINKAGE CGHeroInstance : public CArmedInstance, public IBoatGenerator,
 	friend class CMapLoaderH3M;
 	friend class CMapFormatJson;
 
-private:
 	PrimarySkillsCache primarySkills;
 	MagicSchoolMasteryCache magicSchoolMastery;
 	BonusValueCache manaPerKnowledgeCached;
@@ -163,6 +162,7 @@ public:
 	std::string getClassNameTextID() const;
 
 	bool inBoat() const;
+	CGBoat * getBoat();
 	const CGBoat * getBoat() const;
 	void setBoat(CGBoat * getBoat);
 

lib/mapObjects/CGTownInstance.cpp

@@ -264,8 +264,8 @@ TownFortifications CGTownInstance::fortificationsLevel() const
 }
 
 CGTownInstance::CGTownInstance(IGameCallback *cb):
-	IMarket(cb),
 	CGDwelling(cb),
+	IMarket(cb),
 	built(0),
 	destroyed(0),
 	identifier(0),

lib/mapping/CMapEditManager.cpp

@@ -152,7 +152,7 @@ void CMapEditManager::insertObject(std::shared_ptr<CGObjectInstance> obj)
 	execute(std::make_unique<CInsertObjectOperation>(map, obj));
 }
 
-void CMapEditManager::insertObjects(std::set<std::shared_ptr<CGObjectInstance>>& objects)
+void CMapEditManager::insertObjects(const std::set<std::shared_ptr<CGObjectInstance>>& objects)
 {
 	auto composedOperation = std::make_unique<CComposedOperation>(map);
 	for(auto obj : objects)

lib/mapping/CMapEditManager.h

@@ -84,7 +84,7 @@ public:
 	void drawRiver(RiverId riverType, vstd::RNG * gen);
 
 	void insertObject(std::shared_ptr<CGObjectInstance> obj);
-	void insertObjects(std::set<std::shared_ptr<CGObjectInstance>> & objects);
+	void insertObjects(const std::set<std::shared_ptr<CGObjectInstance>> & objects);
 	void moveObject(CGObjectInstance* obj, const int3 & pos);
 	void removeObject(CGObjectInstance* obj);
 	void removeObjects(std::set<CGObjectInstance*> & objects);

lib/mapping/ObstacleProxy.cpp

@@ -356,7 +356,7 @@ std::set<std::shared_ptr<CGObjectInstance>> ObstacleProxy::createObstacles(vstd:
 
 //FIXME: Only editor placer obstacles directly
 
-void ObstacleProxy::finalInsertion(CMapEditManager * manager, std::set<std::shared_ptr<CGObjectInstance>> & instances)
+void ObstacleProxy::finalInsertion(CMapEditManager * manager, const std::set<std::shared_ptr<CGObjectInstance>> & instances)
 {
 	manager->insertObjects(instances); //insert as one operation - for undo purposes
 }
@@ -368,7 +368,7 @@ std::pair<bool, bool> ObstacleProxy::verifyCoverage(const int3 & t) const
 
 void ObstacleProxy::placeObject(rmg::Object & object, std::set<std::shared_ptr<CGObjectInstance>> & instances)
 {
-	for (auto * instance : object.instances())
+	for (const auto * instance : object.instances())
 	{
 		instances.insert(instance->pointer());
 	}

lib/mapping/ObstacleProxy.h

@@ -47,7 +47,7 @@ public:
 
 	virtual bool isInTheMap(const int3& tile) = 0;
 	
-	void finalInsertion(CMapEditManager * manager, std::set<std::shared_ptr<CGObjectInstance>> & instances);
+	void finalInsertion(CMapEditManager * manager, const std::set<std::shared_ptr<CGObjectInstance>> & instances);
 
 	virtual void postProcess(const rmg::Object& object) {};
 

lib/networkPacks/NetPacksLib.cpp

@@ -1228,7 +1228,7 @@ void RemoveObject::applyGs(CGameState *gs)
 		//If hero on Boat is removed, the Boat disappears
 		if(beatenHero->inBoat())
 		{
-			beatenHero->detachFrom(const_cast<CGBoat&>(*beatenHero->getBoat()));
+			beatenHero->detachFrom(*beatenHero->getBoat());
 			gs->getMap().eraseObject(beatenHero->getBoat()->id);
 			beatenHero->setBoat(nullptr);
 		}
@@ -1327,7 +1327,7 @@ void TryMoveHero::applyGs(CGameState *gs)
 	}
 	else if(result == DISEMBARK) //hero leaves boat to destination tile
 	{
-		auto * b = const_cast<CGBoat *>(h->getBoat());
+		auto * b = h->getBoat();
 		b->direction = h->moveDir;
 		b->pos = start;
 		b->setBoardedHero(nullptr);
@@ -1340,7 +1340,7 @@ void TryMoveHero::applyGs(CGameState *gs)
 	{
 		gs->getMap().removeBlockVisTiles(h);
 		h->pos = end;
-		if(auto * b = const_cast<CGBoat *>(h->getBoat()))
+		if(auto * b = h->getBoat())
 			b->pos = end;
 		gs->getMap().addBlockVisTiles(h);
 	}
@@ -1713,16 +1713,18 @@ void BulkEraseArtifacts::applyGs(CGameState *gs)
 	for(const auto & slot : posPack)
 	{
 		const auto slotInfo = artSet->getSlot(slot);
+		const ArtifactInstanceID artifactID = slotInfo->artifactID;
+		const CArtifactInstance * artifact = gs->getArtInstance(artifactID);
 		if(slotInfo->locked)
 		{
-			logGlobal->debug("Erasing locked artifact: %s", slotInfo->getArt()->getType()->getNameTranslated());
+			logGlobal->debug("Erasing locked artifact: %s", artifact->getType()->getNameTranslated());
 			DisassembledArtifact dis;
 			dis.al.artHolder = artHolder;
 
 			for(auto & slotInfoWorn : artSet->artifactsWorn)
 			{
 				auto art = slotInfoWorn.second.getArt();
-				if(art->isCombined() && art->isPart(slotInfo->getArt()))
+				if(art->isCombined() && art->isPart(artifact))
 				{
 					dis.al.slot = artSet->getArtPos(art);
 					break;
@@ -1734,7 +1736,7 @@ void BulkEraseArtifacts::applyGs(CGameState *gs)
 		}
 		else
 		{
-			logGlobal->debug("Erasing artifact %s", slotInfo->getArt()->getType()->getNameTranslated());
+			logGlobal->debug("Erasing artifact %s", artifact->getType()->getNameTranslated());
 		}
 		gs->getMap().removeArtifactInstance(*artSet, slot);
 	}

lib/pathfinder/CPathfinder.cpp

@@ -498,11 +498,11 @@ int CPathfinderHelper::getGuardiansCount(int3 tile) const
 }
 
 CPathfinderHelper::CPathfinderHelper(CGameState * gs, const CGHeroInstance * Hero, const PathfinderOptions & Options):
-	turn(-1),
 	gs(gs),
+	turn(-1),
+	owner(Hero->tempOwner),
 	hero(Hero),
-	options(Options),
-	owner(Hero->tempOwner)
+	options(Options)
 {
 	turnsInfo.reserve(16);
 	updateTurnInfo();

lib/rewardable/Configuration.h

@@ -12,6 +12,8 @@
 
 #include "Limiter.h"
 #include "Reward.h"
+
+#include "../json/JsonNode.h"
 #include "../networkPacks/EInfoWindowMode.h"
 #include "../texts/MetaString.h"
 

lib/rmg/MapProxy.cpp

@@ -25,7 +25,7 @@ void MapProxy::insertObject(std::shared_ptr<CGObjectInstance> obj)
 	map.getEditManager()->insertObject(obj);
 }
 
-void MapProxy::insertObjects(std::set<std::shared_ptr<CGObjectInstance>>& objects)
+void MapProxy::insertObjects(const std::set<std::shared_ptr<CGObjectInstance>> & objects)
 {
 	Lock lock(mx);
 	map.getEditManager()->insertObjects(objects);

lib/rmg/MapProxy.h

@@ -25,7 +25,7 @@ public:
 	MapProxy(RmgMap & map);
 
 	void insertObject(std::shared_ptr<CGObjectInstance> obj);
-	void insertObjects(std::set<std::shared_ptr<CGObjectInstance>>& objects);
+	void insertObjects(const std::set<std::shared_ptr<CGObjectInstance>> & objects);
 	void removeObject(CGObjectInstance* obj);
 
 	void drawTerrain(vstd::RNG & generator, std::vector<int3> & tiles, TerrainId terrain);

lib/rmg/ObjectInfo.cpp

@@ -21,7 +21,7 @@ VCMI_LIB_NAMESPACE_BEGIN
 ObjectInfo::ObjectInfo(si32 ID, si32 subID):
 	primaryID(ID),
 	secondaryID(subID),
-	destroyObject([](CGObjectInstance & obj){}),
+	destroyObject([](const CGObjectInstance & obj){}),
 	maxPerZone(std::numeric_limits<ui32>::max())
 {
 }

lib/serializer/BinaryDeserializer.h

@@ -16,27 +16,12 @@
 
 VCMI_LIB_NAMESPACE_BEGIN
 
-class DLL_LINKAGE CLoaderBase
-{
-protected:
-	IBinaryReader * reader;
-public:
-	CLoaderBase(IBinaryReader * r): reader(r){};
-
-	inline void read(void * data, unsigned size, bool reverseEndianness)
-	{
-		auto bytePtr = reinterpret_cast<std::byte*>(data);
-
-		reader->read(bytePtr, size);
-		if(reverseEndianness)
-			std::reverse(bytePtr, bytePtr + size);
-	};
-};
-
 /// Main class for deserialization of classes from binary form
 /// Effectively revesed version of BinarySerializer
-class BinaryDeserializer : public CLoaderBase
+class BinaryDeserializer
 {
+	IBinaryReader * reader;
+
 	STRONG_INLINE uint32_t readAndCheckLength()
 	{
 		uint32_t length;
@@ -50,7 +35,14 @@ class BinaryDeserializer : public CLoaderBase
 		return length;
 	}
 
-	int write(const void * data, unsigned size);
+	inline void read(void * data, unsigned size)
+	{
+		auto bytePtr = reinterpret_cast<std::byte*>(data);
+
+		reader->read(bytePtr, size);
+		if(reverseEndianness)
+			std::reverse(bytePtr, bytePtr + size);
+	};
 
 public:
 	using Version = ESerializationVersion;
@@ -72,7 +64,7 @@ public:
 	};
 
 	BinaryDeserializer(IBinaryReader * r)
-		: CLoaderBase(r)
+		: reader(r)
 	{}
 
 	template<class T>
@@ -112,7 +104,7 @@ public:
 	template < class T, typename std::enable_if_t < std::is_floating_point_v<T>, int  > = 0 >
 	void load(T &data)
 	{
-		this->read(static_cast<void *>(&data), sizeof(data), reverseEndianness);
+		this->read(static_cast<void *>(&data), sizeof(data));
 	}
 
 	template < class T, typename std::enable_if_t < std::is_integral_v<T> && !std::is_same_v<T, bool>, int  > = 0 >
@@ -120,7 +112,7 @@ public:
 	{
 		if constexpr (sizeof(T) == 1)
 		{
-			this->read(static_cast<void *>(&data), sizeof(data), reverseEndianness);
+			this->read(static_cast<void *>(&data), sizeof(data));
 		}
 		else
 		{
@@ -147,7 +139,7 @@ public:
 
 	void load(Version &data)
 	{
-		this->read(static_cast<void *>(&data), sizeof(data), reverseEndianness);
+		this->read(static_cast<void *>(&data), sizeof(data));
 	}
 
 	template < typename T, typename std::enable_if_t < std::is_enum_v<T>, int  > = 0 >
@@ -406,7 +398,7 @@ public:
 		if (length > 0)
 		{
 			data.resize(length);
-			this->read(static_cast<void *>(data.data()), length, false);
+			this->read(static_cast<void *>(data.data()), length);
 			loadedStrings.push_back(data);
 		}
 	}

lib/serializer/BinarySerializer.h

@@ -18,26 +18,15 @@
 
 VCMI_LIB_NAMESPACE_BEGIN
 
-class DLL_LINKAGE CSaverBase
-{
-protected:
-	IBinaryWriter * writer;
-public:
-	CSaverBase(IBinaryWriter * w): writer(w){};
-
-	void write(const void * data, unsigned size)
-	{
-		writer->write(reinterpret_cast<const std::byte*>(data), size);
-	};
-};
-
 /// Main class for serialization of classes into binary form
 /// Behaviour for various classes is following:
 /// Primitives:    copy memory into underlying stream (defined in CSaverBase)
 /// Containers:    custom overloaded method that decouples class into primitives
 /// VCMI Classes:  recursively serialize them via ClassName::serialize( BinarySerializer &, int version) call
-class BinarySerializer : public CSaverBase
+class BinarySerializer
 {
+	IBinaryWriter * writer;
+
 	template<typename Handler>
 	struct VariantVisitorSaver
 	{
@@ -53,6 +42,11 @@ class BinarySerializer : public CSaverBase
 		}
 	};
 
+	void write(const void * data, unsigned size)
+	{
+		writer->write(reinterpret_cast<const std::byte*>(data), size);
+	};
+
 public:
 	using Version = ESerializationVersion;
 
@@ -70,7 +64,7 @@ public:
 	};
 
 	BinarySerializer(IBinaryWriter * w)
-		: CSaverBase(w)
+		: writer(w)
 	{
 	}
 

mapeditor/inspector/inspector.cpp

@@ -39,8 +39,8 @@
 
 //===============IMPLEMENT OBJECT INITIALIZATION FUNCTIONS================
 Initializer::Initializer(MapController & controller, CGObjectInstance * o, const PlayerColor & pl)
-	: defaultPlayer(pl)
-	, controller(controller)
+	: controller(controller)
+	, defaultPlayer(pl)
 {
 	logGlobal->info("New object instance initialized");
 ///IMPORTANT! initialize order should be from base objects to derived objects

mapeditor/mapcontroller.cpp

@@ -120,10 +120,9 @@ void MapController::repairMap(CMap * map)
 	for(auto obj : allImpactedObjects)
 	{
 		//fix flags
-		if(obj->getOwner() == PlayerColor::UNFLAGGABLE)
+		if(obj->asOwnable() != nullptr && obj->getOwner() == PlayerColor::UNFLAGGABLE)
 		{
-			if(obj->asOwnable())
-				obj->tempOwner = PlayerColor::NEUTRAL;
+			obj->tempOwner = PlayerColor::NEUTRAL;
 		}
 		//fix hero instance
 		if(auto * nih = dynamic_cast<CGHeroInstance*>(obj))

scripting/lua/api/GameCb.cpp

@@ -32,7 +32,6 @@ const std::vector<GameCbProxy::CustomRegType> GameCbProxy::REGISTER_CUSTOM =
 	{"getPlayer", LuaMethodWrapper<GameCb, decltype(&GameCb::getPlayer), &GameCb::getPlayer>::invoke, false},
 
 	{"getHero", LuaMethodWrapper<GameCb, decltype(&GameCb::getHero), &GameCb::getHero>::invoke, false},
-	{"getHeroWithSubid", LuaMethodWrapper<GameCb, decltype(&GameCb::getHeroWithSubid), &GameCb::getHeroWithSubid>::invoke, false},
 
 	{"getObj", LuaMethodWrapper<GameCb, decltype(&GameCb::getObj), &GameCb::getObj>::invoke, false},
 };

server/CGameHandler.cpp

@@ -550,7 +550,7 @@ void CGameHandler::init(StartInfo *si, Load::ProgressAccumulator & progressTrack
 	gs->init(&mapService, si, progressTracking);
 	logGlobal->info("Gamestate initialized!");
 
-	for (auto & elem : gameState()->players)
+	for (const auto & elem : gameState()->players)
 		turnOrder->addPlayer(elem.first);
 
 //	for (auto & elem : gameState()->getMap().allHeroes)
@@ -646,7 +646,7 @@ void CGameHandler::onNewTurn()
 			}
 		}
 
-		for (auto & elem : gameState()->players)
+		for (const auto & elem : gameState()->players)
 			heroPool->onNewWeek(elem.first);
 
 	}
@@ -3583,10 +3583,10 @@ void CGameHandler::checkVictoryLossConditionsForPlayer(PlayerColor player)
 			std::set<PlayerColor> playerColors;
 
 			//do not copy player state (CBonusSystemNode) by value
-			for (auto &p : gameState()->players) //players may have different colors, iterate over players and not integers
+			for (const auto &playerState : gameState()->players) //players may have different colors, iterate over players and not integers
 			{
-				if (p.first != player)
-					playerColors.insert(p.first);
+				if (playerState.first != player)
+					playerColors.insert(playerState.first);
 			}
 
 			//notify all players

server/queries/VisitQueries.cpp

@@ -52,13 +52,14 @@ MapObjectVisitQuery::MapObjectVisitQuery(CGameHandler * owner, const CGObjectIns
 
 void MapObjectVisitQuery::onRemoval(PlayerColor color)
 {
+	auto object = gh->gameState()->getObjInstance(visitedObject);
 	auto hero = gh->gameState()->getHero(visitingHero);
 
 	gh->objectVisitEnded(hero, players.front());
 
 	//Can object visit affect 2 players and what would be desired behavior?
 	if(removeObjectAfterVisit)
-		gh->removeObject(hero, color);
+		gh->removeObject(object, color);
 }
 
 TownBuildingVisitQuery::TownBuildingVisitQuery(CGameHandler * owner, const CGTownInstance * Obj, std::vector<const CGHeroInstance *> heroes, std::vector<BuildingID> buildingToVisit)