Proper fix for #1712 - Building requirement tests are now fixed

- added minimize() method in logical expressions that will remove excessive elements
- replaced forEach() with morph() that allows replacing types in variant

client/CCastleInterface.cpp

@@ -1358,13 +1358,9 @@ std::string CBuildWindow::getTextForState(int state)
 			{
 				return town->town->buildings.at(build)->Name();
 			};
-			/*auto toBool = [&](const BuildingID build)
-			{
-				return town->hasBuilt(build);
-			};*/
 
 			ret = CGI->generaltexth->allTexts[52];
-			ret += "\n" + building->requirements.toString(toStr);
+			ret += "\n" + town->genBuildingRequirements(building->bid).toString(toStr);
 			break;
 		}
 	case EBuildingState::MISSING_BASE:

lib/CGameInfoCallback.cpp

@@ -403,33 +403,12 @@ EBuildingState::EBuildingState CGameInfoCallback::canBuildStructure( const CGTow
 			return EBuildingState::NO_WATER; //lack of water
 	}
 
-	/// returns true if building prerequisites are fulfilled
-	std::function<bool(BuildingID)> buildTest;
-
-	auto dependTest = [&](BuildingID id) -> bool
-	{
-		const CBuilding * build = t->town->buildings.at(id);
-
-		if (build->upgrade != BuildingID::NONE)
-		{
-			if (!t->hasBuilt(build->upgrade))
-				return false;
-
-			if (!t->town->buildings.at(build->upgrade)->requirements.test(buildTest))
-				return false;
-		}
-
-		if (!build->requirements.test(buildTest))
-			return false;
-		return true;
-	};
-
-	buildTest = [&](BuildingID bid)
+	auto buildTest = [&](BuildingID id) -> bool
 	{
-		return t->hasBuilt(bid) && dependTest(bid);
+		return t->hasBuilt(id);
 	};
 
-	if (!dependTest(ID))
+	if (!t->genBuildingRequirements(ID).test(buildTest))
 		return EBuildingState::PREREQUIRES;
 
 	if(t->builded >= VLC->modh->settings.MAX_BUILDING_PER_TURN)

lib/LogicalExpression.h

@@ -45,6 +45,11 @@ namespace LogicalExpressionDetail
 
 			std::vector<Variant> expressions;
 
+			bool operator == (const Element & other) const
+			{
+				return expressions == other.expressions;
+			}
+
 			template <typename Handler>
 			void serialize(Handler & h, const int version)
 			{
@@ -147,39 +152,73 @@ namespace LogicalExpressionDetail
 
 	/// Simple foreach visitor
 	template <typename ContainedClass>
-	class ForEachVisitor : public boost::static_visitor<void>
+	class ForEachVisitor : public boost::static_visitor<typename ExpressionBase<ContainedClass>::Variant>
 	{
 		typedef ExpressionBase<ContainedClass> Base;
 
-		std::function<void(typename Base::Value &)> visitor;
+		std::function<typename Base::Variant(const typename Base::Value &)> visitor;
 
 	public:
-		ForEachVisitor(std::function<void(typename Base::Value &)> visitor):
+		ForEachVisitor(std::function<typename Base::Variant(const typename Base::Value &)> visitor):
 			visitor(visitor)
 		{}
 
-		//FIXME: duplicated code
-		void operator()(typename Base::OperatorAny & element) const
+		typename Base::Variant operator()(const typename Base::Value & element) const
 		{
-			for (auto & entry : element.expressions)
-				boost::apply_visitor(*this, entry);
+			return visitor(element);
 		}
 
-		void operator()(typename Base::OperatorAll & element) const
+		template <typename Type>
+		typename Base::Variant operator()(Type element) const
 		{
 			for (auto & entry : element.expressions)
-				boost::apply_visitor(*this, entry);
+				entry = boost::apply_visitor(*this, entry);
+			return element;
 		}
+	};
 
-		void operator()(typename Base::OperatorNone & element) const
+	/// Minimizing visitor that removes all redundant elements from variant (e.g. AllOf inside another AllOf can be merged safely)
+	template <typename ContainedClass>
+	class MinimizingVisitor : public boost::static_visitor<typename ExpressionBase<ContainedClass>::Variant>
+	{
+		typedef ExpressionBase<ContainedClass> Base;
+
+	public:
+		typename Base::Variant operator()(const typename Base::Value & element) const
 		{
-			for (auto & entry : element.expressions)
-				boost::apply_visitor(*this, entry);
+			return element;
 		}
 
-		void operator()(typename Base::Value & element) const
+		template <typename Type>
+		typename Base::Variant operator()(const Type & element) const
 		{
-			visitor(element);
+			Type ret;
+
+			for (auto & entryRO : element.expressions)
+			{
+				auto entry = boost::apply_visitor(*this, entryRO);
+
+				try
+				{
+					// copy entries from child of this type
+					auto sublist = boost::get<Type>(entry).expressions;
+					std::move(sublist.begin(), sublist.end(), std::back_inserter(ret.expressions));
+				}
+				catch (boost::bad_get &)
+				{
+					// different type (e.g. allOf vs oneOf) just copy
+					ret.expressions.push_back(entry);
+				}
+			}
+
+			for ( auto it = ret.expressions.begin(); it != ret.expressions.end();)
+			{
+				if (std::find(ret.expressions.begin(), it, *it) != it)
+					it = ret.expressions.erase(it); // erase duplicate
+				else
+					it++; // goto next
+			}
+			return ret;
 		}
 	};
 
@@ -370,16 +409,23 @@ public:
 		std::swap(data, expr.data);
 	}
 
-	Variant get()
+	Variant get() const
 	{
 		return data;
 	}
 
 	/// Simple visitor that visits all entries in expression
-	void forEach(std::function<void(Value &)> visitor)
+	Variant morph(std::function<Variant(const Value &)> morpher) const
+	{
+		LogicalExpressionDetail::ForEachVisitor<Value> visitor(morpher);
+		return boost::apply_visitor(visitor, data);
+	}
+
+	/// Minimizes expression, removing any redundant elements
+	void minimize()
 	{
-		LogicalExpressionDetail::ForEachVisitor<Value> testVisitor(visitor);
-		boost::apply_visitor(testVisitor, data);
+		LogicalExpressionDetail::MinimizingVisitor<Value> visitor;
+		data = boost::apply_visitor(visitor, data);
 	}
 
 	/// calculates if expression evaluates to "true".

lib/NetPacksLib.cpp

@@ -396,7 +396,7 @@ DLL_LINKAGE void RemoveObject::applyGs( CGameState *gs )
 
 	for (TriggeredEvent & event : gs->map->triggeredEvents)
 	{
-		auto patcher = [&](EventCondition & cond)
+		auto patcher = [&](EventCondition cond) -> EventExpression::Variant
 		{
 			if (cond.object == obj)
 			{
@@ -411,8 +411,9 @@ DLL_LINKAGE void RemoveObject::applyGs( CGameState *gs )
 					cond.value = 0; // destroyed object, from now on can not be fulfilled
 				}
 			}
+			return cond;
 		};
-		event.trigger.forEach(patcher);
+		event.trigger = event.trigger.morph(patcher);
 	}
 
 	gs->map->objects[id.getNum()].dellNull();

lib/mapObjects/CGTownInstance.cpp

@@ -1004,6 +1004,40 @@ bool CGTownInstance::hasBuilt(BuildingID buildingID) const
 	return vstd::contains(builtBuildings, buildingID);
 }
 
+CBuilding::TRequired CGTownInstance::genBuildingRequirements(BuildingID buildID) const
+{
+	const CBuilding * building = town->buildings.at(buildID);
+
+	std::function<CBuilding::TRequired::Variant(const BuildingID &)> dependTest =
+	[&](const BuildingID & id) -> CBuilding::TRequired::Variant
+	{
+		const CBuilding * build = town->buildings.at(id);
+
+		if (!hasBuilt(id))
+			return id;
+
+		if (build->upgrade != BuildingID::NONE && !hasBuilt(build->upgrade))
+			return build->upgrade;
+
+		return build->requirements.morph(dependTest);
+	};
+
+	CBuilding::TRequired::OperatorAll requirements;
+	if (building->upgrade != BuildingID::NONE)
+	{
+		const CBuilding * upgr = town->buildings.at(building->upgrade);
+
+		requirements.expressions.push_back(upgr->bid);
+		requirements.expressions.push_back(upgr->requirements.morph(dependTest));
+	}
+	requirements.expressions.push_back(building->requirements.morph(dependTest));
+
+	CBuilding::TRequired::Variant variant(requirements);
+	CBuilding::TRequired ret(variant);
+	ret.minimize();
+	return ret;
+}
+
 void CGTownInstance::addHeroToStructureVisitors( const CGHeroInstance *h, si32 structureInstanceID ) const
 {
 	if(visitingHero == h)

lib/mapObjects/CGTownInstance.h

@@ -232,6 +232,8 @@ public:
 	bool armedGarrison() const; //true if town has creatures in garrison or garrisoned hero
 	int getTownLevel() const;
 
+	CBuilding::TRequired genBuildingRequirements(BuildingID build) const;
+
 	void removeCapitols (PlayerColor owner) const;
 	void addHeroToStructureVisitors(const CGHeroInstance *h, si32 structureInstanceID) const; //hero must be visiting or garrisoned in town
 

lib/mapping/CMap.cpp

@@ -441,7 +441,7 @@ void CMap::checkForObjectives()
 	// NOTE: probably should be moved to MapFormatH3M.cpp
 	for (TriggeredEvent & event : triggeredEvents)
 	{
-		auto patcher = [&](EventCondition & cond)
+		auto patcher = [&](EventCondition cond) -> EventExpression::Variant
 		{
 			switch (cond.condition)
 			{
@@ -491,8 +491,9 @@ void CMap::checkForObjectives()
 				//break; case EventCondition::DAYS_WITHOUT_TOWN:
 				//break; case EventCondition::STANDARD_WIN:
 			}
+			return cond;
 		};
-		event.trigger.forEach(patcher);
+		event.trigger = event.trigger.morph(patcher);
 	}
 }
 

lib/mapping/MapFormatH3M.cpp

@@ -676,16 +676,17 @@ void CMapLoaderH3M::readAllowedArtifacts()
 	// Messy, but needed
 	for (TriggeredEvent & event : map->triggeredEvents)
 	{
-		auto patcher = [&](EventCondition & cond)
+		auto patcher = [&](EventCondition cond) -> EventExpression::Variant
 		{
 			if (cond.condition == EventCondition::HAVE_ARTIFACT ||
 				cond.condition == EventCondition::TRANSPORT)
 			{
 				map->allowedArtifact[cond.objectType] = false;
 			}
+			return cond;
 		};
 
-		event.trigger.forEach(patcher);
+		event.trigger = event.trigger.morph(patcher);
 	}
 }