Split map object visit from town building visit. Removes side effects

from building visit

config/factions/dungeon.json

@@ -179,7 +179,6 @@
 							"period" : 7,
 							"visitors" : true
 						},
-						"visitMode" : "hero", // Should be 'once' to match (somewhat buggy) H3 logic
 						"visitMode" : "once",
 						"rewards" : [
 							{

server/CGameHandler.cpp

@@ -2178,8 +2178,7 @@ bool CGameHandler::visitTownBuilding(ObjectInstanceID tid, BuildingID bid)
 
 		if (hero && t->town->buildings.at(bid)->manualHeroVisit)
 		{
-			// FIXME: query might produce unintended side effects, double check
-			auto visitQuery = std::make_shared<CObjectVisitQuery>(this, t, hero);
+			auto visitQuery = std::make_shared<TownBuildingVisitQuery>(this, t, hero, bid);
 			queries->addQuery(visitQuery);
 			building->onHeroVisit(hero);
 			queries->popIfTop(visitQuery);
@@ -3208,7 +3207,7 @@ void CGameHandler::objectVisited(const CGObjectInstance * obj, const CGHeroInsta
 		throw std::runtime_error("Can not visit object that is being visited");
 	}
 
-	std::shared_ptr<CObjectVisitQuery> visitQuery;
+	std::shared_ptr<MapObjectVisitQuery> visitQuery;
 
 	auto startVisit = [&](ObjectVisitStarted & event)
 	{
@@ -3227,7 +3226,7 @@ void CGameHandler::objectVisited(const CGObjectInstance * obj, const CGHeroInsta
 					visitedObject = visitedTown;
 			}
 		}
-		visitQuery = std::make_shared<CObjectVisitQuery>(this, visitedObject, h);
+		visitQuery = std::make_shared<MapObjectVisitQuery>(this, visitedObject, h);
 		queries->addQuery(visitQuery); //TODO real visit pos
 
 		HeroVisit hv;
@@ -3246,11 +3245,11 @@ void CGameHandler::objectVisited(const CGObjectInstance * obj, const CGHeroInsta
 		queries->popIfTop(visitQuery); //visit ends here if no queries were created
 }
 
-void CGameHandler::objectVisitEnded(const CObjectVisitQuery & query)
+void CGameHandler::objectVisitEnded(const CGHeroInstance *h, PlayerColor player)
 {
 	using events::ObjectVisitEnded;
 
-	logGlobal->debug("%s visit ends.\n", query.visitingHero->nodeName());
+	logGlobal->debug("%s visit ends.\n", h->nodeName());
 
 	auto endVisit = [&](ObjectVisitEnded & event)
 	{
@@ -3263,7 +3262,7 @@ void CGameHandler::objectVisitEnded(const CObjectVisitQuery & query)
 
 	//TODO: ObjectVisitEnded should also have id of visited object,
 	//but this requires object being deleted only by `removeAfterVisit()` but not `removeObject()`
-	ObjectVisitEnded::defaultExecute(serverEventBus.get(), endVisit, query.players.front(), query.visitingHero->id);
+	ObjectVisitEnded::defaultExecute(serverEventBus.get(), endVisit, player, h->id);
 }
 
 bool CGameHandler::buildBoat(ObjectInstanceID objid, PlayerColor playerID)
@@ -3874,7 +3873,7 @@ void CGameHandler::removeAfterVisit(const CGObjectInstance *object)
 	//If the object is being visited, there must be a matching query
 	for (const auto &query : queries->allQueries())
 	{
-		if (auto someVistQuery = std::dynamic_pointer_cast<CObjectVisitQuery>(query))
+		if (auto someVistQuery = std::dynamic_pointer_cast<MapObjectVisitQuery>(query))
 		{
 			if (someVistQuery->visitedObject == object)
 			{
@@ -3938,7 +3937,7 @@ const CGHeroInstance * CGameHandler::getVisitingHero(const CGObjectInstance *obj
 
 	for(const auto & query : queries->allQueries())
 	{
-		auto visit = std::dynamic_pointer_cast<const CObjectVisitQuery>(query);
+		auto visit = std::dynamic_pointer_cast<const VisitQuery>(query);
 		if (visit && visit->visitedObject == obj)
 			return visit->visitingHero;
 	}
@@ -3951,7 +3950,7 @@ const CGObjectInstance * CGameHandler::getVisitingObject(const CGHeroInstance *h
 
 	for(const auto & query : queries->allQueries())
 	{
-		auto visit = std::dynamic_pointer_cast<const CObjectVisitQuery>(query);
+		auto visit = std::dynamic_pointer_cast<const VisitQuery>(query);
 		if (visit && visit->visitingHero == hero)
 			return visit->visitedObject;
 	}
@@ -3968,7 +3967,7 @@ bool CGameHandler::isVisitCoveredByAnotherQuery(const CGObjectInstance *obj, con
 	// visitation query is covered by other query that must be answered first
 
 	if (auto topQuery = queries->topQuery(hero->getOwner()))
-		if (auto visit = std::dynamic_pointer_cast<const CObjectVisitQuery>(topQuery))
+		if (auto visit = std::dynamic_pointer_cast<const VisitQuery>(topQuery))
 			return !(visit->visitedObject == obj && visit->visitingHero == hero);
 
 	return true;

server/CGameHandler.h

@@ -232,7 +232,7 @@ public:
 
 	bool complain(const std::string &problem); //sends message to all clients, prints on the logs and return true
 	void objectVisited( const CGObjectInstance * obj, const CGHeroInstance * h );
-	void objectVisitEnded(const CObjectVisitQuery &query);
+	void objectVisitEnded(const CGHeroInstance *h, PlayerColor player);
 	bool dig(const CGHeroInstance *h);
 	void moveArmy(const CArmedInstance *src, const CArmedInstance *dst, bool allowMerging);
 

server/queries/VisitQueries.cpp

@@ -10,26 +10,43 @@
 #include "StdInc.h"
 #include "VisitQueries.h"
 
-#include "QueriesProcessor.h"
-#include "../CGameHandler.h"
 #include "../../lib/mapObjects/CGHeroInstance.h"
+#include "../CGameHandler.h"
+#include "QueriesProcessor.h"
 
-CObjectVisitQuery::CObjectVisitQuery(CGameHandler * owner, const CGObjectInstance * Obj, const CGHeroInstance * Hero):
-	CQuery(owner), visitedObject(Obj), visitingHero(Hero), removeObjectAfterVisit(false)
+VisitQuery::VisitQuery(CGameHandler * owner, const CGObjectInstance * Obj, const CGHeroInstance * Hero)
+	: CQuery(owner)
+	, visitedObject(Obj)
+	, visitingHero(Hero)
 {
 	addPlayer(Hero->tempOwner);
 }
 
-bool CObjectVisitQuery::blocksPack(const CPack *pack) const
+bool VisitQuery::blocksPack(const CPack * pack) const
 {
 	//During the visit itself ALL actions are blocked.
 	//(However, the visit may trigger a query above that'll pass some.)
 	return true;
 }
 
-void CObjectVisitQuery::onRemoval(PlayerColor color)
+void VisitQuery::onExposure(QueryPtr topQuery)
+{
+	//Object may have been removed and deleted.
+	if(gh->isValidObject(visitedObject))
+		topQuery->notifyObjectAboutRemoval(visitedObject, visitingHero);
+
+	owner->popIfTop(*this);
+}
+
+MapObjectVisitQuery::MapObjectVisitQuery(CGameHandler * owner, const CGObjectInstance * Obj, const CGHeroInstance * Hero)
+	: VisitQuery(owner, Obj, Hero)
+	, removeObjectAfterVisit(false)
+{
+}
+
+void MapObjectVisitQuery::onRemoval(PlayerColor color)
 {
-	gh->objectVisitEnded(*this);
+	gh->objectVisitEnded(visitingHero, players.front());
 
 	//TODO or should it be destructor?
 	//Can object visit affect 2 players and what would be desired behavior?
@@ -37,11 +54,13 @@ void CObjectVisitQuery::onRemoval(PlayerColor color)
 		gh->removeObject(visitedObject, color);
 }
 
-void CObjectVisitQuery::onExposure(QueryPtr topQuery)
+TownBuildingVisitQuery::TownBuildingVisitQuery(CGameHandler * owner, const CGObjectInstance * Obj, const CGHeroInstance * Hero, BuildingID buildingToVisit)
+	: VisitQuery(owner, Obj, Hero)
+	, visitedBuilding(buildingToVisit)
+{
+}
+
+void TownBuildingVisitQuery::onRemoval(PlayerColor color)
 {
-	//Object may have been removed and deleted.
-	if(gh->isValidObject(visitedObject))
-		topQuery->notifyObjectAboutRemoval(visitedObject, visitingHero);
 
-	owner->popIfTop(*this);
 }

server/queries/VisitQueries.h

@@ -13,17 +13,35 @@
 
 //Created when hero visits object.
 //Removed when query above is resolved (or immediately after visit if no queries were created)
-class CObjectVisitQuery : public CQuery
+class VisitQuery : public CQuery
 {
+protected:
+	VisitQuery(CGameHandler * owner, const CGObjectInstance *Obj, const CGHeroInstance *Hero);
+
 public:
 	const CGObjectInstance *visitedObject;
 	const CGHeroInstance *visitingHero;
 
+	bool blocksPack(const CPack *pack) const final;
+	void onExposure(QueryPtr topQuery) final;
+};
+
+class MapObjectVisitQuery final : public VisitQuery
+{
+public:
 	bool removeObjectAfterVisit;
 
-	CObjectVisitQuery(CGameHandler * owner, const CGObjectInstance *Obj, const CGHeroInstance *Hero);
+	MapObjectVisitQuery(CGameHandler * owner, const CGObjectInstance *Obj, const CGHeroInstance *Hero);
+
+	void onRemoval(PlayerColor color) final;
+};
+
+class TownBuildingVisitQuery final : public VisitQuery
+{
+public:
+	BuildingID visitedBuilding;
+
+	TownBuildingVisitQuery(CGameHandler * owner, const CGObjectInstance *Obj, const CGHeroInstance *Hero, BuildingID buildingToVisit);
 
-	bool blocksPack(const CPack *pack) const override;
-	void onRemoval(PlayerColor color) override;
-	void onExposure(QueryPtr topQuery) override;
+	void onRemoval(PlayerColor color) final;
 };