- Move Hero / Prison distribution to separate modificator

- Protect rolling and banning hero with mutex

cmake_modules/VCMI_lib.cmake

@@ -156,6 +156,7 @@ macro(add_main_lib TARGET_NAME LIBRARY_TYPE)
 		${MAIN_LIB_DIR}/rmg/modificators/ObjectDistributor.cpp
 		${MAIN_LIB_DIR}/rmg/modificators/RoadPlacer.cpp
 		${MAIN_LIB_DIR}/rmg/modificators/TreasurePlacer.cpp
+		${MAIN_LIB_DIR}/rmg/modificators/PrisonHeroPlacer.cpp
 		${MAIN_LIB_DIR}/rmg/modificators/QuestArtifactPlacer.cpp
 		${MAIN_LIB_DIR}/rmg/modificators/ConnectionsPlacer.cpp
 		${MAIN_LIB_DIR}/rmg/modificators/WaterAdopter.cpp
@@ -526,6 +527,7 @@ macro(add_main_lib TARGET_NAME LIBRARY_TYPE)
 		${MAIN_LIB_DIR}/rmg/modificators/ObjectDistributor.h
 		${MAIN_LIB_DIR}/rmg/modificators/RoadPlacer.h
 		${MAIN_LIB_DIR}/rmg/modificators/TreasurePlacer.h
+		${MAIN_LIB_DIR}/rmg/modificators/PrisonHeroPlacer.h
 		${MAIN_LIB_DIR}/rmg/modificators/QuestArtifactPlacer.h
 		${MAIN_LIB_DIR}/rmg/modificators/ConnectionsPlacer.h
 		${MAIN_LIB_DIR}/rmg/modificators/WaterAdopter.h

lib/mapping/CMap.cpp

@@ -649,6 +649,8 @@ void CMap::banWaterHeroes()
 
 void CMap::banHero(const HeroTypeID & id)
 {
+	if (!vstd::contains(allowedHeroes, id))
+		logGlobal->warn("Attempt to ban hero %d, who is already not allowed", id.encode(id));
 	allowedHeroes.erase(id);
 }
 

lib/rmg/CMapGenerator.cpp

@@ -488,6 +488,7 @@ const std::vector<HeroTypeID> CMapGenerator::getAllPossibleHeroes() const
 	auto isWaterMap = map->getMap(this).isWaterMap();
 	//Skip heroes that were banned, including the ones placed in prisons
 	std::vector<HeroTypeID> ret;
+
 	for (HeroTypeID hero : map->getMap(this).allowedHeroes)
 	{
 		auto * h = dynamic_cast<const CHero*>(VLC->heroTypes()->getById(hero));
@@ -505,13 +506,11 @@ const std::vector<HeroTypeID> CMapGenerator::getAllPossibleHeroes() const
 
 void CMapGenerator::banQuestArt(const ArtifactID & id)
 {
-	//TODO: Protect with mutex
 	map->getMap(this).allowedArtifact.erase(id);
 }
 
 void CMapGenerator::banHero(const HeroTypeID & id)
 {
-	//TODO: Protect with mutex
 	map->getMap(this).banHero(id);
 }
 

lib/rmg/RmgMap.cpp

@@ -19,6 +19,7 @@
 #include "modificators/ObjectManager.h"
 #include "modificators/RoadPlacer.h"
 #include "modificators/TreasurePlacer.h"
+#include "modificators/PrisonHeroPlacer.h"
 #include "modificators/QuestArtifactPlacer.h"
 #include "modificators/ConnectionsPlacer.h"
 #include "modificators/TownPlacer.h"
@@ -122,6 +123,7 @@ void RmgMap::initTiles(CMapGenerator & generator, CRandomGenerator & rand)
 void RmgMap::addModificators()
 {
 	bool hasObjectDistributor = false;
+	bool hasHeroPlacer = false;
 	bool hasRockFiller = false;
 
 	for(auto & z : getZones())
@@ -134,6 +136,11 @@ void RmgMap::addModificators()
 			zone->addModificator<ObjectDistributor>();
 			hasObjectDistributor = true;
 		}
+		if (!hasHeroPlacer)
+		{
+			zone->addModificator<PrisonHeroPlacer>();
+			hasHeroPlacer = true;
+		}
 		zone->addModificator<TreasurePlacer>();
 		zone->addModificator<ObstaclePlacer>();
 		zone->addModificator<TerrainPainter>();

lib/rmg/modificators/ObjectDistributor.cpp

@@ -75,7 +75,6 @@ void ObjectDistributor::distributeLimitedObjects()
 
 					auto rmgInfo = handler->getRMGInfo();
 
-					// FIXME: Random order of distribution
 					RandomGeneratorUtil::randomShuffle(matchingZones, zone.getRand());
 					for (auto& zone : matchingZones)
 					{

lib/rmg/modificators/PrisonHeroPlacer.cpp

@@ -0,0 +1,56 @@
+/*
+* PrisonHeroPlacer.cpp, part of VCMI engine
+*
+* Authors: listed in file AUTHORS in main folder
+*
+* License: GNU General Public License v2.0 or later
+* Full text of license available in license.txt file, in main folder
+*
+*/
+
+#include "StdInc.h"
+#include "PrisonHeroPlacer.h"
+#include "../CMapGenerator.h"
+#include "../RmgMap.h"
+#include "TreasurePlacer.h"
+#include "../CZonePlacer.h"
+#include "../../VCMI_Lib.h"
+#include "../../mapObjectConstructors/AObjectTypeHandler.h"
+#include "../../mapObjectConstructors/CObjectClassesHandler.h"
+#include "../../mapObjects/MapObjects.h" 
+
+VCMI_LIB_NAMESPACE_BEGIN
+
+void PrisonHeroPlacer::process()
+{
+	getAllowedHeroes();
+}
+
+void PrisonHeroPlacer::init()
+{
+}
+
+void PrisonHeroPlacer::getAllowedHeroes()
+{
+    allowedHeroes = generator.getAllPossibleHeroes();
+}
+
+HeroTypeID PrisonHeroPlacer::drawRandomHero()
+{
+	RecursiveLock lock(externalAccessMutex);
+	if (!allowedHeroes.empty())
+	{
+		RandomGeneratorUtil::randomShuffle(allowedHeroes, zone.getRand());
+        HeroTypeID ret = allowedHeroes.back();
+        allowedHeroes.pop_back();
+
+        generator.banHero(ret);
+		return ret;
+	}
+	else
+	{
+		throw rmgException("No quest heroes left for prisons!");
+	}
+}
+
+VCMI_LIB_NAMESPACE_END

lib/rmg/modificators/PrisonHeroPlacer.h

@@ -0,0 +1,40 @@
+/*
+* PrisonHeroPlacer, part of VCMI engine
+*
+* Authors: listed in file AUTHORS in main folder
+*
+* License: GNU General Public License v2.0 or later
+* Full text of license available in license.txt file, in main folder
+*
+*/
+
+#pragma once
+#include "../Zone.h"
+#include "../Functions.h"
+#include "../../mapObjects/ObjectTemplate.h"
+
+VCMI_LIB_NAMESPACE_BEGIN
+
+class CRandomGenerator;
+
+class PrisonHeroPlacer : public Modificator
+{
+public:
+	MODIFICATOR(PrisonHeroPlacer);
+
+	void process() override;
+	void init() override;
+
+	HeroTypeID drawRandomHero();
+
+private:
+    void getAllowedHeroes();
+
+protected:
+
+    std::vector<HeroTypeID> allowedHeroes;
+
+	// TODO: Count allowed heroes?
+};
+
+VCMI_LIB_NAMESPACE_END

lib/rmg/modificators/QuestArtifactPlacer.cpp

@@ -131,9 +131,10 @@ ArtifactID QuestArtifactPlacer::drawRandomArtifact()
 	RecursiveLock lock(externalAccessMutex);
 	if (!questArtifacts.empty())
 	{
+		RandomGeneratorUtil::randomShuffle(questArtifacts, zone.getRand());
 		ArtifactID ret = questArtifacts.back();
 		questArtifacts.pop_back();
-		RandomGeneratorUtil::randomShuffle(questArtifacts, zone.getRand());
+		generator.banQuestArt(ret);
 		return ret;
 	}
 	else

lib/rmg/modificators/TreasurePlacer.cpp

@@ -18,6 +18,7 @@
 #include "../RmgMap.h"
 #include "../TileInfo.h"
 #include "../CZonePlacer.h"
+#include "PrisonHeroPlacer.h"
 #include "QuestArtifactPlacer.h"
 #include "../../ArtifactUtils.h"
 #include "../../mapObjectConstructors/AObjectTypeHandler.h"
@@ -45,6 +46,7 @@ void TreasurePlacer::init()
 	maxPrisons = 0; //Should be in the constructor, but we use macro for that
 	DEPENDENCY(ObjectManager);
 	DEPENDENCY(ConnectionsPlacer);
+	DEPENDENCY_ALL(PrisonHeroPlacer);
 	POSTFUNCTION(RoadPlacer);
 }
 
@@ -90,6 +92,15 @@ void TreasurePlacer::addAllPossibleObjects()
 	auto prisonTemplates = VLC->objtypeh->getHandlerFor(Obj::PRISON, 0)->getTemplates(zone.getTerrainType());
 	if (!prisonTemplates.empty())
 	{
+		PrisonHeroPlacer * prisonHeroPlacer = nullptr;
+		for(auto & z : map.getZones())
+		{
+		 	if (prisonHeroPlacer = z.second->getModificator<PrisonHeroPlacer>())
+			{
+				break;
+			}
+		}
+
 		//prisons
 		//levels 1, 5, 10, 20, 30
 		static int prisonsLevels = std::min(generator.getConfig().prisonExperience.size(), generator.getConfig().prisonValues.size());
@@ -103,10 +114,11 @@ void TreasurePlacer::addAllPossibleObjects()
 				continue;
 			}
 
-			oi.generateObject = [i, this]() -> CGObjectInstance*
+			oi.generateObject = [i, this, prisonHeroPlacer]() -> CGObjectInstance*
 			{
 				auto possibleHeroes = generator.getAllPossibleHeroes();
-				HeroTypeID hid = *RandomGeneratorUtil::nextItem(possibleHeroes, zone.getRand());
+
+				HeroTypeID hid = prisonHeroPlacer->drawRandomHero();
 
 				auto factory = VLC->objtypeh->getHandlerFor(Obj::PRISON, 0);
 				auto* obj = dynamic_cast<CGHeroInstance*>(factory->create());
@@ -114,7 +126,6 @@ void TreasurePlacer::addAllPossibleObjects()
 				obj->setHeroType(hid); //will be initialized later
 				obj->exp = generator.getConfig().prisonExperience[i];
 				obj->setOwner(PlayerColor::NEUTRAL);
-				generator.banHero(hid);
 
 				return obj;
 			};
@@ -464,7 +475,6 @@ void TreasurePlacer::addAllPossibleObjects()
 				ArtifactID artid = qap->drawRandomArtifact();
 				obj->quest->mission.artifacts.push_back(artid);
 				
-				generator.banQuestArt(artid);
 				zone.getModificator<QuestArtifactPlacer>()->addQuestArtifact(artid);
 				
 				return obj;
@@ -512,7 +522,6 @@ void TreasurePlacer::addAllPossibleObjects()
 				ArtifactID artid = qap->drawRandomArtifact();
 				obj->quest->mission.artifacts.push_back(artid);
 				
-				generator.banQuestArt(artid);
 				zone.getModificator<QuestArtifactPlacer>()->addQuestArtifact(artid);
 				
 				return obj;
@@ -534,7 +543,6 @@ void TreasurePlacer::addAllPossibleObjects()
 				ArtifactID artid = qap->drawRandomArtifact();
 				obj->quest->mission.artifacts.push_back(artid);
 				
-				generator.banQuestArt(artid);
 				zone.getModificator<QuestArtifactPlacer>()->addQuestArtifact(artid);
 				
 				return obj;
@@ -631,8 +639,14 @@ rmg::Object TreasurePlacer::constructTreasurePile(const std::vector<ObjectInfo*>
 			entrableArea.add(int3());
 		
 		auto * object = oi->generateObject();
+
+		// FIXME: Possible memory leak, but this is a weird case in first place
 		if(oi->templates.empty())
+		{
+			logGlobal->warn("Deleting randomized object with no templates: %s", object->getObjectName());
+			delete object; // FIXME: We also lose randomized hero or quest artifact
 			continue;
+		}
 		
 		object->appearance = *RandomGeneratorUtil::nextItem(oi->templates, zone.getRand());
 
@@ -695,7 +709,7 @@ rmg::Object TreasurePlacer::constructTreasurePile(const std::vector<ObjectInfo*>
 					instanceAccessibleArea.add(instance.getVisitablePosition());
 			}
 			
-			//first object is good
+			//Do not clean up after first object
 			if(rmgObject.instances().size() == 1)
 				break;
 
@@ -777,7 +791,6 @@ void TreasurePlacer::createTreasures(ObjectManager& manager)
 			oi->maxPerZone++;
 		}
 	};
-
 	//place biggest treasures first at large distance, place smaller ones inbetween
 	auto treasureInfo = zone.getTreasureInfo();
 	boost::sort(treasureInfo, valueComparator);