Fix code review suggestions

AI/Nullkiller/Goals/CompleteQuest.cpp

@@ -38,26 +38,26 @@ TGoalVec CompleteQuest::decompose() const
 
 	logAi->debug("Trying to realize quest: %s", questToString());
 	
-	if(!q.quest->artifacts.empty())
+	if(!q.quest->mission.artifacts.empty())
 		return missionArt();
 
-	if(!q.quest->heroes.empty())
+	if(!q.quest->mission.heroes.empty())
 		return missionHero();
 
-	if(!q.quest->creatures.empty())
+	if(!q.quest->mission.creatures.empty())
 		return missionArmy();
 
-	if(q.quest->resources.nonZero())
+	if(q.quest->mission.resources.nonZero())
 		return missionResources();
 
-	if(q.quest->killTarget >= 0)
+	if(q.quest->killTarget != ObjectInstanceID::NONE)
 		return missionDestroyObj();
 
-	for(auto & s : q.quest->primary)
+	for(auto & s : q.quest->mission.primary)
 		if(s)
 			return missionIncreasePrimaryStat();
 
-	if(q.quest->heroLevel > 0)
+	if(q.quest->mission.heroLevel > 0)
 		return missionLevel();
 	
 	if(q.quest->questName == CQuest::missionName(10))
@@ -127,7 +127,7 @@ TGoalVec CompleteQuest::missionArt() const
 
 	CaptureObjectsBehavior findArts;
 
-	for(auto art : q.quest->artifacts)
+	for(auto art : q.quest->mission.artifacts)
 	{
 		solutions.push_back(sptr(CaptureObjectsBehavior().ofType(Obj::ARTIFACT, art)));
 	}

AI/Nullkiller/Pathfinding/Rules/AIMovementAfterDestinationRule.cpp

@@ -130,7 +130,9 @@ namespace AIPathfinding
 		auto questInfo = QuestInfo(questObj->quest, destination.nodeObject, destination.coord);
 		QuestAction questAction(questInfo);
 
-		if(destination.nodeObject->ID == Obj::QUEST_GUARD && questObj->quest->empty() && questObj->quest->killTarget == -1)
+		if(destination.nodeObject->ID == Obj::QUEST_GUARD
+		   && questObj->quest->mission == Rewardable::Limiter{}
+		   && questObj->quest->killTarget == ObjectInstanceID::NONE)
 		{
 			return false;
 		}

AI/VCAI/Goals/CompleteQuest.cpp

@@ -29,26 +29,26 @@ TGoalVec CompleteQuest::getAllPossibleSubgoals()
 	{
 		logAi->debug("Trying to realize quest: %s", questToString());
 
-		if(!q.quest->artifacts.empty())
+		if(!q.quest->mission.artifacts.empty())
 			return missionArt();
 
-		if(!q.quest->heroes.empty())
+		if(!q.quest->mission.heroes.empty())
 			return missionHero();
 
-		if(!q.quest->creatures.empty())
+		if(!q.quest->mission.creatures.empty())
 			return missionArmy();
 
-		if(q.quest->resources.nonZero())
+		if(q.quest->mission.resources.nonZero())
 			return missionResources();
 
-		if(q.quest->killTarget >= 0)
+		if(q.quest->killTarget != ObjectInstanceID::NONE)
 			return missionDestroyObj();
 
-		for(auto & s : q.quest->primary)
+		for(auto & s : q.quest->mission.primary)
 			if(s)
 				return missionIncreasePrimaryStat();
 
-		if(q.quest->heroLevel > 0)
+		if(q.quest->mission.heroLevel > 0)
 			return missionLevel();
 		
 		if(q.quest->questName == CQuest::missionName(10))
@@ -127,7 +127,7 @@ TGoalVec CompleteQuest::missionArt() const
 	if(!solutions.empty())
 		return solutions;
 
-	for(auto art : q.quest->artifacts)
+	for(auto art : q.quest->mission.artifacts)
 	{
 		solutions.push_back(sptr(GetArtOfType(art))); //TODO: transport?
 	}
@@ -155,7 +155,7 @@ TGoalVec CompleteQuest::missionArmy() const
 	if(!solutions.empty())
 		return solutions;
 
-	for(auto creature : q.quest->creatures)
+	for(auto creature : q.quest->mission.creatures)
 	{
 		solutions.push_back(sptr(GatherTroops(creature.type->getId(), creature.count)));
 	}
@@ -169,7 +169,7 @@ TGoalVec CompleteQuest::missionIncreasePrimaryStat() const
 
 	if(solutions.empty())
 	{
-		for(int i = 0; i < q.quest->primary.size(); ++i)
+		for(int i = 0; i < q.quest->mission.primary.size(); ++i)
 		{
 			// TODO: library, school and other boost objects
 			logAi->debug("Don't know how to increase primary stat %d", i);
@@ -185,7 +185,7 @@ TGoalVec CompleteQuest::missionLevel() const
 
 	if(solutions.empty())
 	{
-		logAi->debug("Don't know how to reach hero level %d", q.quest->heroLevel);
+		logAi->debug("Don't know how to reach hero level %d", q.quest->mission.heroLevel);
 	}
 
 	return solutions;
@@ -217,10 +217,10 @@ TGoalVec CompleteQuest::missionResources() const
 		}
 		else
 		{
-			for(int i = 0; i < q.quest->resources.size(); ++i)
+			for(int i = 0; i < q.quest->mission.resources.size(); ++i)
 			{
-				if(q.quest->resources[i])
-					solutions.push_back(sptr(CollectRes(static_cast<EGameResID>(i), q.quest->resources[i])));
+				if(q.quest->mission.resources[i])
+					solutions.push_back(sptr(CollectRes(static_cast<EGameResID>(i), q.quest->mission.resources[i])));
 			}
 		}
 	}

docs/modders/Map_Objects/Rewardable.md

@@ -456,7 +456,6 @@ Keep in mind, that all randomization is performed on map load and on object rese
 - Can be used as limiter
 - Can NOT be used as reward
 - Only players with specific color can pass the limiter
-- If not specified or empty all colors may pass the limiter
 
 ```jsonc
 "colors" : [ "red", "blue", "tan", "green", "orange", "purple", "teal", "pink" ]

lib/CCreatureSet.cpp

@@ -1029,6 +1029,14 @@ void CStackBasicDescriptor::setType(const CCreature * c)
 	type = c;
 }
 
+bool operator== (const CStackBasicDescriptor & l, const CStackBasicDescriptor & r)
+{
+	return (!l.type && !r.type)
+	|| (l.type && r.type
+		&& l.type->getId() == r.type->getId()
+		&& l.count == r.count);
+}
+
 void CStackBasicDescriptor::serializeJson(JsonSerializeFormat & handler)
 {
 	handler.serializeInt("amount", count);

lib/CCreatureSet.h

@@ -42,6 +42,8 @@ public:
 	TQuantity getCount() const;
 
 	virtual void setType(const CCreature * c);
+	
+	friend bool operator== (const CStackBasicDescriptor & l, const CStackBasicDescriptor & r);
 
 	template <typename Handler> void serialize(Handler &h, const int version)
 	{

lib/mapObjects/CQuest.cpp

@@ -40,7 +40,7 @@ CQuest::CQuest():
 	qid(-1),
 	progress(NOT_ACTIVE),
 	lastDay(-1),
-	killTarget(-1),
+	killTarget(ObjectInstanceID::NONE),
 	textOption(0),
 	completedOption(0),
 	stackDirection(0),
@@ -101,7 +101,7 @@ bool CQuest::checkMissionArmy(const CQuest * q, const CCreatureSet * army)
 	ui32 count = 0;
 	ui32 slotsCount = 0;
 	bool hasExtraCreatures = false;
-	for(cre = q->creatures.begin(); cre != q->creatures.end(); ++cre)
+	for(cre = q->mission.creatures.begin(); cre != q->mission.creatures.end(); ++cre)
 	{
 		for(count = 0, it = army->Slots().begin(); it != army->Slots().end(); ++it)
 		{
@@ -123,10 +123,10 @@ bool CQuest::checkMissionArmy(const CQuest * q, const CCreatureSet * army)
 
 bool CQuest::checkQuest(const CGHeroInstance * h) const
 {
-	if(!heroAllowed(h))
+	if(!mission.heroAllowed(h))
 		return false;
 	
-	if(killTarget >= 0)
+	if(killTarget != ObjectInstanceID::NONE)
 	{
 		if(CGHeroInstance::cb->getObjByQuestIdentifier(killTarget))
 			return false;
@@ -137,7 +137,7 @@ bool CQuest::checkQuest(const CGHeroInstance * h) const
 
 void CQuest::completeQuest(IGameCallback * cb, const CGHeroInstance *h) const
 {
-	for(auto & elem : artifacts)
+	for(auto & elem : mission.artifacts)
 	{
 		if(h->hasArt(elem))
 		{
@@ -161,36 +161,36 @@ void CQuest::completeQuest(IGameCallback * cb, const CGHeroInstance *h) const
 		}
 	}
 			
-	cb->takeCreatures(h->id, creatures);
-	cb->giveResources(h->getOwner(), resources);
+	cb->takeCreatures(h->id, mission.creatures);
+	cb->giveResources(h->getOwner(), mission.resources);
 }
 
 void CQuest::addTextReplacements(MetaString & text, std::vector<Component> & components) const
 {
-	if(heroLevel > 0)
-		text.replaceNumber(heroLevel);
+	if(mission.heroLevel > 0)
+		text.replaceNumber(mission.heroLevel);
 	
-	if(heroExperience > 0)
-		text.replaceNumber(heroExperience);
+	if(mission.heroExperience > 0)
+		text.replaceNumber(mission.heroExperience);
 	
 	{ //primary skills
 		MetaString loot;
 		for(int i = 0; i < 4; ++i)
 		{
-			if(primary[i])
+			if(mission.primary[i])
 			{
 				loot.appendRawString("%d %s");
-				loot.replaceNumber(primary[i]);
+				loot.replaceNumber(mission.primary[i]);
 				loot.replaceRawString(VLC->generaltexth->primarySkillNames[i]);
 			}
 		}
 		
-		for(auto & skill : secondary)
+		for(auto & skill : mission.secondary)
 		{
 			loot.appendTextID(VLC->skillh->getById(skill.first)->getNameTextID());
 		}
 		
-		for(auto & spell : spells)
+		for(auto & spell : mission.spells)
 		{
 			loot.appendTextID(VLC->spellh->getById(spell)->getNameTextID());
 		}
@@ -199,25 +199,25 @@ void CQuest::addTextReplacements(MetaString & text, std::vector<Component> & com
 			text.replaceRawString(loot.buildList());
 	}
 	
-	if(killTarget >= 0 && !heroName.empty())
+	if(killTarget != ObjectInstanceID::NONE && !heroName.empty())
 	{
 		components.emplace_back(Component::EComponentType::HERO_PORTRAIT, heroPortrait, 0, 0);
 		addKillTargetReplacements(text);
 	}
 	
-	if(killTarget >= 0 && stackToKill.type)
+	if(killTarget != ObjectInstanceID::NONE && stackToKill.type)
 	{
 		components.emplace_back(stackToKill);
 		addKillTargetReplacements(text);
 	}
 	
-	if(!heroes.empty())
-		text.replaceRawString(VLC->heroh->getById(heroes.front())->getNameTranslated());
+	if(!mission.heroes.empty())
+		text.replaceRawString(VLC->heroh->getById(mission.heroes.front())->getNameTranslated());
 	
-	if(!artifacts.empty())
+	if(!mission.artifacts.empty())
 	{
 		MetaString loot;
-		for(const auto & elem : artifacts)
+		for(const auto & elem : mission.artifacts)
 		{
 			loot.appendRawString("%s");
 			loot.replaceLocalString(EMetaText::ART_NAMES, elem);
@@ -225,10 +225,10 @@ void CQuest::addTextReplacements(MetaString & text, std::vector<Component> & com
 		text.replaceRawString(loot.buildList());
 	}
 	
-	if(!creatures.empty())
+	if(!mission.creatures.empty())
 	{
 		MetaString loot;
-		for(const auto & elem : creatures)
+		for(const auto & elem : mission.creatures)
 		{
 			loot.appendRawString("%s");
 			loot.replaceCreatureName(elem);
@@ -236,25 +236,25 @@ void CQuest::addTextReplacements(MetaString & text, std::vector<Component> & com
 		text.replaceRawString(loot.buildList());
 	}
 	
-	if(resources.nonZero())
+	if(mission.resources.nonZero())
 	{
 		MetaString loot;
 		for(int i = 0; i < 7; ++i)
 		{
-			if(resources[i])
+			if(mission.resources[i])
 			{
 				loot.appendRawString("%d %s");
-				loot.replaceNumber(resources[i]);
+				loot.replaceNumber(mission.resources[i]);
 				loot.replaceLocalString(EMetaText::RES_NAMES, i);
 			}
 		}
 		text.replaceRawString(loot.buildList());
 	}
 	
-	if(!players.empty())
+	if(!mission.players.empty())
 	{
 		MetaString loot;
-		for(auto & p : players)
+		for(auto & p : mission.players)
 			loot.appendLocalString(EMetaText::COLOR, p);
 		
 		text.replaceRawString(loot.buildList());
@@ -264,7 +264,7 @@ void CQuest::addTextReplacements(MetaString & text, std::vector<Component> & com
 void CQuest::getVisitText(MetaString &iwText, std::vector<Component> &components, bool firstVisit, const CGHeroInstance * h) const
 {
 	bool failRequirements = (h ? !checkQuest(h) : true);
-	loadComponents(components, h);
+	mission.loadComponents(components, h);
 
 	if(firstVisit)
 		iwText.appendRawString(firstVisitText.toString());
@@ -299,17 +299,17 @@ void CQuest::defineQuestName()
 {
 	//standard quests
 	questName = CQuest::missionName(0);
-	if(heroLevel > 0) questName = CQuest::missionName(1);
-	for(auto & s : primary) if(s) questName = CQuest::missionName(2);
-	if(!spells.empty()) questName = CQuest::missionName(2);
-	if(!secondary.empty()) questName = CQuest::missionName(2);
-	if(killTarget >= 0 && !heroName.empty()) questName = CQuest::missionName(3);
-	if(killTarget >= 0 && stackToKill.getType()) questName = CQuest::missionName(4);
-	if(!artifacts.empty()) questName = CQuest::missionName(5);
-	if(!creatures.empty()) questName = CQuest::missionName(6);
-	if(resources.nonZero()) questName = CQuest::missionName(7);
-	if(!heroes.empty()) questName = CQuest::missionName(8);
-	if(!players.empty()) questName = CQuest::missionName(9);
+	if(mission.heroLevel > 0) questName = CQuest::missionName(1);
+	for(auto & s : mission.primary) if(s) questName = CQuest::missionName(2);
+	if(!mission.spells.empty()) questName = CQuest::missionName(2);
+	if(!mission.secondary.empty()) questName = CQuest::missionName(2);
+	if(killTarget != ObjectInstanceID::NONE && !heroName.empty()) questName = CQuest::missionName(3);
+	if(killTarget != ObjectInstanceID::NONE && stackToKill.getType()) questName = CQuest::missionName(4);
+	if(!mission.artifacts.empty()) questName = CQuest::missionName(5);
+	if(!mission.creatures.empty()) questName = CQuest::missionName(6);
+	if(mission.resources.nonZero()) questName = CQuest::missionName(7);
+	if(!mission.heroes.empty()) questName = CQuest::missionName(8);
+	if(!mission.players.empty()) questName = CQuest::missionName(9);
 }
 
 void CQuest::addKillTargetReplacements(MetaString &out) const
@@ -339,10 +339,9 @@ void CQuest::serializeJson(JsonSerializeFormat & handler, const std::string & fi
 		isCustomComplete = !completedText.empty();
 	}
 	
-	Rewardable::Limiter::serializeJson(handler);
-
 	handler.serializeInt("timeLimit", lastDay, -1);
-	handler.serializeInstance<int>("killTarget", killTarget, -1);
+	handler.serializeStruct("limiter", mission);
+	handler.serializeInstance("killTarget", killTarget, ObjectInstanceID::NONE);
 
 	if(!handler.saving) //compatibility with legacy vmaps
 	{
@@ -352,22 +351,22 @@ void CQuest::serializeJson(JsonSerializeFormat & handler, const std::string & fi
 			return;
 		
 		if(missionType == "Level")
-			handler.serializeInt("heroLevel", heroLevel, -1);
+			handler.serializeInt("heroLevel", mission.heroLevel, -1);
 		
 		if(missionType == "PrimaryStat")
 		{
 			auto primarySkills = handler.enterStruct("primarySkills");
 			for(int i = 0; i < GameConstants::PRIMARY_SKILLS; ++i)
-				handler.serializeInt(NPrimarySkill::names[i], primary[i], 0);
+				handler.serializeInt(NPrimarySkill::names[i], mission.primary[i], 0);
 		}
 		
 		if(missionType == "Artifact")
-			handler.serializeIdArray<ArtifactID>("artifacts", artifacts);
+			handler.serializeIdArray<ArtifactID>("artifacts", mission.artifacts);
 		
 		if(missionType == "Army")
 		{
 			auto a = handler.enterArray("creatures");
-			a.serializeStruct(creatures);
+			a.serializeStruct(mission.creatures);
 		}
 		
 		if(missionType == "Resources")
@@ -376,7 +375,7 @@ void CQuest::serializeJson(JsonSerializeFormat & handler, const std::string & fi
 
 			for(size_t idx = 0; idx < (GameConstants::RESOURCE_QUANTITY - 1); idx++)
 			{
-				handler.serializeInt(GameConstants::RESOURCE_NAMES[idx], resources[idx], 0);
+				handler.serializeInt(GameConstants::RESOURCE_NAMES[idx], mission.resources[idx], 0);
 			}
 		}
 		
@@ -384,14 +383,14 @@ void CQuest::serializeJson(JsonSerializeFormat & handler, const std::string & fi
 		{
 			ui32 temp;
 			handler.serializeId<ui32, ui32, HeroTypeID>("hero", temp, 0);
-			heroes.emplace_back(temp);
+			mission.heroes.emplace_back(temp);
 		}
 		
 		if(missionType == "Player")
 		{
 			ui32 temp;
 			handler.serializeId<ui32, ui32, PlayerColor>("player", temp, PlayerColor::NEUTRAL);
-			players.emplace_back(temp);
+			mission.players.emplace_back(temp);
 		}
 	}
 
@@ -457,7 +456,7 @@ void CGSeerHut::initObj(CRandomGenerator & rand)
 	setObjToKill();
 	quest->defineQuestName();
 	
-	if(quest->empty() && quest->killTarget == -1)
+	if(quest->mission == Rewardable::Limiter{} && quest->killTarget == ObjectInstanceID::NONE)
 	   quest->progress = CQuest::COMPLETE;
 	
 	if(quest->questName == quest->missionName(0))
@@ -504,7 +503,7 @@ void CGSeerHut::setPropertyDer (ui8 what, ui32 val)
 {
 	switch(what)
 	{
-		case 10:
+		case CGSeerHut::OBJPROP_VISITED:
 			quest->progress = static_cast<CQuest::EProgress>(val);
 			break;
 	}

lib/mapObjects/CQuest.h

@@ -17,7 +17,7 @@ VCMI_LIB_NAMESPACE_BEGIN
 
 class CGCreature;
 
-class DLL_LINKAGE CQuest: public Rewardable::Limiter
+class DLL_LINKAGE CQuest final
 {
 public:
 
@@ -36,7 +36,8 @@ public:
 
 	EProgress progress;
 	si32 lastDay; //after this day (first day is 0) mission cannot be completed; if -1 - no limit
-	int killTarget;
+	ObjectInstanceID killTarget;
+	Rewardable::Limiter mission;
 	bool repeatedQuest;
 
 	// following fields are used only for kill creature/hero missions, the original
@@ -89,7 +90,8 @@ public:
 		h & isCustomComplete;
 		h & completedOption;
 		h & questName;
-		h & static_cast<Rewardable::Limiter&>(*this);
+		h & mission;
+		h & killTarget;
 	}
 
 	void serializeJson(JsonSerializeFormat & handler, const std::string & fieldName);

lib/mapping/MapFormatH3M.cpp

@@ -1890,7 +1890,7 @@ void CMapLoaderH3M::readSeerHutQuest(CGSeerHut * hut, const int3 & position, con
 		if(artID != ArtifactID::NONE)
 		{
 			//not none quest
-			hut->quest->artifacts.push_back(artID);
+			hut->quest->mission.artifacts.push_back(artID);
 			missionType = EQuestMission::ARTIFACT;
 		}
 		hut->quest->lastDay = -1; //no timeout
@@ -2003,19 +2003,19 @@ int CMapLoaderH3M::readQuest(IQuestObject * guard, const int3 & position)
 		{
 			for(int x = 0; x < 4; ++x)
 			{
-				guard->quest->primary[x] = reader->readUInt8();
+				guard->quest->mission.primary[x] = reader->readUInt8();
 			}
 			break;
 		}
 		case EQuestMission::LEVEL:
 		{
-			guard->quest->heroLevel = reader->readUInt32();
+			guard->quest->mission.heroLevel = reader->readUInt32();
 			break;
 		}
 		case EQuestMission::KILL_HERO:
 		case EQuestMission::KILL_CREATURE:
 		{
-			guard->quest->killTarget = reader->readUInt32();
+			guard->quest->killTarget = ObjectInstanceID(reader->readUInt32());
 			break;
 		}
 		case EQuestMission::ARTIFACT:
@@ -2024,7 +2024,7 @@ int CMapLoaderH3M::readQuest(IQuestObject * guard, const int3 & position)
 			for(int yy = 0; yy < artNumber; ++yy)
 			{
 				auto artid = reader->readArtifact();
-				guard->quest->artifacts.push_back(artid);
+				guard->quest->mission.artifacts.push_back(artid);
 				map->allowedArtifact[artid] = false; //these are unavailable for random generation
 			}
 			break;
@@ -2032,29 +2032,29 @@ int CMapLoaderH3M::readQuest(IQuestObject * guard, const int3 & position)
 		case EQuestMission::ARMY:
 		{
 			int typeNumber = reader->readUInt8();
-			guard->quest->creatures.resize(typeNumber);
+			guard->quest->mission.creatures.resize(typeNumber);
 			for(int hh = 0; hh < typeNumber; ++hh)
 			{
-				guard->quest->creatures[hh].type = VLC->creh->objects[reader->readCreature()];
-				guard->quest->creatures[hh].count = reader->readUInt16();
+				guard->quest->mission.creatures[hh].type = VLC->creh->objects[reader->readCreature()];
+				guard->quest->mission.creatures[hh].count = reader->readUInt16();
 			}
 			break;
 		}
 		case EQuestMission::RESOURCES:
 		{
 			for(int x = 0; x < 7; ++x)
-				guard->quest->resources[x] = reader->readUInt32();
+				guard->quest->mission.resources[x] = reader->readUInt32();
 
 			break;
 		}
 		case EQuestMission::HERO:
 		{
-			guard->quest->heroes.push_back(reader->readHero());
+			guard->quest->mission.heroes.push_back(reader->readHero());
 			break;
 		}
 		case EQuestMission::PLAYER:
 		{
-			guard->quest->players.push_back(reader->readPlayer());
+			guard->quest->mission.players.push_back(reader->readPlayer());
 			break;
 		}
 		case EQuestMission::HOTA_MULTI:
@@ -2067,13 +2067,13 @@ int CMapLoaderH3M::readQuest(IQuestObject * guard, const int3 & position)
 				std::set<HeroClassID> heroClasses;
 				reader->readBitmaskHeroClassesSized(heroClasses, false);
 				for(auto & hc : heroClasses)
-					guard->quest->heroClasses.push_back(hc);
+					guard->quest->mission.heroClasses.push_back(hc);
 				break;
 			}
 			if(missionSubID == 1)
 			{
 				missionId = int(EQuestMission::HOTA_REACH_DATE);
-				guard->quest->daysPassed = reader->readUInt32();
+				guard->quest->mission.daysPassed = reader->readUInt32();
 				break;
 			}
 			break;

lib/rewardable/Info.cpp

@@ -112,7 +112,7 @@ void Rewardable::Info::configureLimiter(Rewardable::Configuration & object, CRan
 	limiter.heroLevel = JsonRandom::loadValue(source["heroLevel"], rng)
 					 + JsonRandom::loadValue(source["minLevel"], rng); // VCMI 1.1 compatibilty
 
-	limiter.manaPercentage = JsonRandom::loadValue(source["manaPercentage"], rng);
+	limiter.manaPercentage = JsonRandom::loadValue(source["manaPercentage"], rng, -1);
 	limiter.manaPoints = JsonRandom::loadValue(source["manaPoints"], rng);
 
 	limiter.resources = JsonRandom::loadResources(source["resources"], rng);

lib/rewardable/Limiter.cpp

@@ -27,7 +27,7 @@ Rewardable::Limiter::Limiter()
 	, daysPassed(0)
 	, heroExperience(0)
 	, heroLevel(-1)
-	, manaPercentage(0)
+	, manaPercentage(-1)
 	, manaPoints(0)
 	, primary(GameConstants::PRIMARY_SKILLS, 0)
 {
@@ -35,35 +35,26 @@ Rewardable::Limiter::Limiter()
 
 Rewardable::Limiter::~Limiter() = default;
 
-bool Rewardable::Limiter::empty() const
+bool operator==(const Rewardable::Limiter & l, const Rewardable::Limiter & r)
 {
-	if(dayOfWeek != 0
-	   || daysPassed != 0
-	   || heroLevel > 0
-	   || heroExperience > 0
-	   || manaPoints > 0
-	   || manaPercentage > 0
-	   || !secondary.empty()
-	   || !creatures.empty()
-	   || !spells.empty()
-	   || !artifacts.empty()
-	   || !players.empty()
-	   || !heroes.empty()
-	   || !heroClasses.empty()
-	   || resources.nonZero()
-	   || std::find_if(primary.begin(), primary.end(), [](si32 i){return i != 0;}) != primary.end())
-		return false;
-	
-	for(const auto & sub : {noneOf, allOf, anyOf})
-	{
-		for(const auto & sublimiter : sub)
-		{
-			if(!sublimiter->empty())
-				return false;
-		}
-	}
-	
-	return true;
+	return l.dayOfWeek == r.dayOfWeek
+	&& l.daysPassed == r.daysPassed
+	&& l.heroLevel == r.heroLevel
+	&& l.heroExperience == r.heroExperience
+	&& l.manaPoints == r.manaPoints
+	&& l.manaPercentage == r.manaPercentage
+	&& l.secondary == r.secondary
+	&& l.creatures == r.creatures
+	&& l.spells == r.spells
+	&& l.artifacts == r.artifacts
+	&& l.players == r.players
+	&& l.heroes == r.heroes
+	&& l.heroClasses == r.heroClasses
+	&& l.resources == r.resources
+	&& l.primary == r.primary
+	&& l.noneOf == r.noneOf
+	&& l.allOf == r.allOf
+	&& l.anyOf == r.anyOf;
 }
 
 bool Rewardable::Limiter::heroAllowed(const CGHeroInstance * hero) const
@@ -127,7 +118,7 @@ bool Rewardable::Limiter::heroAllowed(const CGHeroInstance * hero) const
 	}
 
 	{
-		std::unordered_map<ArtifactID, unsigned, ArtifactID::hash> artifactsRequirements; // artifact ID -> required count
+		std::unordered_map<ArtifactID, unsigned int, ArtifactID::hash> artifactsRequirements; // artifact ID -> required count
 		for(const auto & art : artifacts)
 			++artifactsRequirements[art];
 		
@@ -186,9 +177,9 @@ void Rewardable::Limiter::loadComponents(std::vector<Component> & comps,
 	if (heroLevel)
 		comps.emplace_back(Component::EComponentType::EXPERIENCE, 1, heroLevel, 0);
 
-	if (manaPoints || manaPercentage > 0)
+	if (manaPoints || manaPercentage >= 0)
 	{
-		int absoluteMana = h->manaLimit() ? (manaPercentage * h->mana / h->manaLimit() / 100) : 0;
+		int absoluteMana = (h->manaLimit() && manaPercentage >= 0) ? (manaPercentage * h->mana / h->manaLimit() / 100) : 0;
 		comps.emplace_back(Component::EComponentType::PRIM_SKILL, 5, absoluteMana + manaPoints, 0);
 	}
 
@@ -232,7 +223,7 @@ void Rewardable::Limiter::serializeJson(JsonSerializeFormat & handler)
 	handler.serializeInt("dayOfWeek", dayOfWeek);
 	handler.serializeInt("daysPassed", daysPassed);
 	resources.serializeJson(handler, "resources");
-	handler.serializeInt("manaPercentage", manaPercentage);
+	handler.serializeInt("manaPercentage", manaPercentage, -1);
 	handler.serializeInt("heroExperience", heroExperience);
 	handler.serializeInt("heroLevel", heroLevel);
 	handler.serializeIdArray("heroes", heroes);

lib/rewardable/Limiter.h

@@ -26,7 +26,7 @@ using LimitersList = std::vector<std::shared_ptr<Rewardable::Limiter>>;
 
 /// Limiters of rewards. Rewards will be granted to hero only if he satisfies requirements
 /// Note: for this is only a test - it won't remove anything from hero (e.g. artifacts or creatures)
-struct DLL_LINKAGE Limiter
+struct DLL_LINKAGE Limiter final
 {
 	/// day of week, unused if 0, 1-7 will test for current day of week
 	si32 dayOfWeek;
@@ -81,7 +81,6 @@ struct DLL_LINKAGE Limiter
 	virtual ~Limiter();
 
 	bool heroAllowed(const CGHeroInstance * hero) const;
-	bool empty() const;
 	
 	/// Generates list of components that describes reward for a specific hero
 	virtual void loadComponents(std::vector<Component> & comps,
@@ -100,12 +99,12 @@ struct DLL_LINKAGE Limiter
 		h & secondary;
 		h & artifacts;
 		h & creatures;
-		h & allOf;
-		h & anyOf;
-		h & noneOf;
 		h & heroes;
 		h & heroClasses;
 		h & players;
+		h & allOf;
+		h & anyOf;
+		h & noneOf;
 	}
 	
 	void serializeJson(JsonSerializeFormat & handler);
@@ -113,4 +112,6 @@ struct DLL_LINKAGE Limiter
 
 }
 
+bool DLL_LINKAGE operator== (const Rewardable::Limiter & l, const Rewardable::Limiter & r);
+
 VCMI_LIB_NAMESPACE_END

lib/rewardable/Reward.h

@@ -29,7 +29,7 @@ using RewardsList = std::vector<std::shared_ptr<Rewardable::Reward>>;
 
 /// Reward that can be granted to a hero
 /// NOTE: eventually should replace seer hut rewards and events/pandoras
-struct DLL_LINKAGE Reward
+struct DLL_LINKAGE Reward final
 {
 	/// resources that will be given to player
 	TResources resources;
@@ -86,7 +86,7 @@ struct DLL_LINKAGE Reward
 	si32 calculateManaPoints(const CGHeroInstance * h) const;
 
 	Reward();
-	virtual ~Reward();
+	~Reward();
 
 	template <typename Handler> void serialize(Handler &h, const int version)
 	{

lib/rmg/modificators/TreasurePlacer.cpp

@@ -462,7 +462,7 @@ void TreasurePlacer::addAllPossibleObjects()
 				obj->configuration.info.push_back(reward);
 								
 				ArtifactID artid = qap->drawRandomArtifact();
-				obj->quest->artifacts.push_back(artid);
+				obj->quest->mission.artifacts.push_back(artid);
 				
 				generator.banQuestArt(artid);
 				zone.getModificator<QuestArtifactPlacer>()->addQuestArtifact(artid);
@@ -510,7 +510,7 @@ void TreasurePlacer::addAllPossibleObjects()
 				obj->configuration.info.push_back(reward);
 				
 				ArtifactID artid = qap->drawRandomArtifact();
-				obj->quest->artifacts.push_back(artid);
+				obj->quest->mission.artifacts.push_back(artid);
 				
 				generator.banQuestArt(artid);
 				zone.getModificator<QuestArtifactPlacer>()->addQuestArtifact(artid);
@@ -532,7 +532,7 @@ void TreasurePlacer::addAllPossibleObjects()
 				obj->configuration.info.push_back(reward);
 				
 				ArtifactID artid = qap->drawRandomArtifact();
-				obj->quest->artifacts.push_back(artid);
+				obj->quest->mission.artifacts.push_back(artid);
 				
 				generator.banQuestArt(artid);
 				zone.getModificator<QuestArtifactPlacer>()->addQuestArtifact(artid);

mapeditor/inspector/questwidget.cpp

@@ -137,40 +137,40 @@ QuestWidget::~QuestWidget()
 
 void QuestWidget::obtainData()
 {
-	ui->lDayOfWeek->setCurrentIndex(quest.dayOfWeek);
-	ui->lDaysPassed->setValue(quest.daysPassed);
-	ui->lHeroLevel->setValue(quest.heroLevel);
-	ui->lHeroExperience->setValue(quest.heroExperience);
-	ui->lManaPoints->setValue(quest.manaPoints);
-	ui->lManaPercentage->setValue(quest.manaPercentage);
-	ui->lAttack->setValue(quest.primary[0]);
-	ui->lDefence->setValue(quest.primary[1]);
-	ui->lPower->setValue(quest.primary[2]);
-	ui->lKnowledge->setValue(quest.primary[3]);
+	ui->lDayOfWeek->setCurrentIndex(quest.mission.dayOfWeek);
+	ui->lDaysPassed->setValue(quest.mission.daysPassed);
+	ui->lHeroLevel->setValue(quest.mission.heroLevel);
+	ui->lHeroExperience->setValue(quest.mission.heroExperience);
+	ui->lManaPoints->setValue(quest.mission.manaPoints);
+	ui->lManaPercentage->setValue(quest.mission.manaPercentage);
+	ui->lAttack->setValue(quest.mission.primary[0]);
+	ui->lDefence->setValue(quest.mission.primary[1]);
+	ui->lPower->setValue(quest.mission.primary[2]);
+	ui->lKnowledge->setValue(quest.mission.primary[3]);
 	for(int i = 0; i < ui->lResources->rowCount(); ++i)
 	{
 		if(auto * widget = qobject_cast<QSpinBox*>(ui->lResources->cellWidget(i, 1)))
-			widget->setValue(quest.resources[i]);
+			widget->setValue(quest.mission.resources[i]);
 	}
 	
-	for(auto i : quest.artifacts)
+	for(auto i : quest.mission.artifacts)
 		ui->lArtifacts->item(VLC->artifacts()->getById(i)->getIndex())->setCheckState(Qt::Checked);
-	for(auto i : quest.spells)
+	for(auto i : quest.mission.spells)
 		ui->lArtifacts->item(VLC->spells()->getById(i)->getIndex())->setCheckState(Qt::Checked);
-	for(auto & i : quest.secondary)
+	for(auto & i : quest.mission.secondary)
 	{
 		int index = VLC->skills()->getById(i.first)->getIndex();
 		if(auto * widget = qobject_cast<QComboBox*>(ui->lSkills->cellWidget(index, 1)))
 			widget->setCurrentIndex(i.second);
 	}
-	for(auto & i : quest.creatures)
+	for(auto & i : quest.mission.creatures)
 	{
 		int index = i.type->getIndex();
 		ui->lCreatureId->setCurrentIndex(index);
 		ui->lCreatureAmount->setValue(i.count);
 		onCreatureAdd(ui->lCreatures, ui->lCreatureId, ui->lCreatureAmount);
 	}
-	for(auto & i : quest.heroes)
+	for(auto & i : quest.mission.heroes)
 	{
 		for(int e = 0; e < ui->lHeroes->count(); ++e)
 		{
@@ -181,7 +181,7 @@ void QuestWidget::obtainData()
 			}
 		}
 	}
-	for(auto & i : quest.heroClasses)
+	for(auto & i : quest.mission.heroClasses)
 	{
 		for(int e = 0; e < ui->lHeroClasses->count(); ++e)
 		{
@@ -192,7 +192,7 @@ void QuestWidget::obtainData()
 			}
 		}
 	}
-	for(auto & i : quest.players)
+	for(auto & i : quest.mission.players)
 	{
 		for(int e = 0; e < ui->lPlayers->count(); ++e)
 		{
@@ -204,81 +204,81 @@ void QuestWidget::obtainData()
 		}
 	}
 	
-	if(quest.killTarget >= 0 && quest.killTarget < controller.map()->objects.size())
+	if(quest.killTarget != ObjectInstanceID::NONE && quest.killTarget < controller.map()->objects.size())
 		ui->lKillTarget->setText(QString::fromStdString(controller.map()->objects[quest.killTarget]->instanceName));
 	else
-		quest.killTarget = -1;
+		quest.killTarget = ObjectInstanceID::NONE;
 }
 
 bool QuestWidget::commitChanges()
 {
-	quest.dayOfWeek = ui->lDayOfWeek->currentIndex();
-	quest.daysPassed = ui->lDaysPassed->value();
-	quest.heroLevel = ui->lHeroLevel->value();
-	quest.heroExperience = ui->lHeroExperience->value();
-	quest.manaPoints = ui->lManaPoints->value();
-	quest.manaPercentage = ui->lManaPercentage->value();
-	quest.primary[0] = ui->lAttack->value();
-	quest.primary[1] = ui->lDefence->value();
-	quest.primary[2] = ui->lPower->value();
-	quest.primary[3] = ui->lKnowledge->value();
+	quest.mission.dayOfWeek = ui->lDayOfWeek->currentIndex();
+	quest.mission.daysPassed = ui->lDaysPassed->value();
+	quest.mission.heroLevel = ui->lHeroLevel->value();
+	quest.mission.heroExperience = ui->lHeroExperience->value();
+	quest.mission.manaPoints = ui->lManaPoints->value();
+	quest.mission.manaPercentage = ui->lManaPercentage->value();
+	quest.mission.primary[0] = ui->lAttack->value();
+	quest.mission.primary[1] = ui->lDefence->value();
+	quest.mission.primary[2] = ui->lPower->value();
+	quest.mission.primary[3] = ui->lKnowledge->value();
 	for(int i = 0; i < ui->lResources->rowCount(); ++i)
 	{
 		if(auto * widget = qobject_cast<QSpinBox*>(ui->lResources->cellWidget(i, 1)))
-			quest.resources[i] = widget->value();
+			quest.mission.resources[i] = widget->value();
 	}
 	
-	quest.artifacts.clear();
+	quest.mission.artifacts.clear();
 	for(int i = 0; i < ui->lArtifacts->count(); ++i)
 	{
 		if(ui->lArtifacts->item(i)->checkState() == Qt::Checked)
-			quest.artifacts.push_back(VLC->artifacts()->getByIndex(i)->getId());
+			quest.mission.artifacts.push_back(VLC->artifacts()->getByIndex(i)->getId());
 	}
-	quest.spells.clear();
+	quest.mission.spells.clear();
 	for(int i = 0; i < ui->lSpells->count(); ++i)
 	{
 		if(ui->lSpells->item(i)->checkState() == Qt::Checked)
-			quest.spells.push_back(VLC->spells()->getByIndex(i)->getId());
+			quest.mission.spells.push_back(VLC->spells()->getByIndex(i)->getId());
 	}
 	
-	quest.secondary.clear();
+	quest.mission.secondary.clear();
 	for(int i = 0; i < ui->lSkills->rowCount(); ++i)
 	{
 		if(auto * widget = qobject_cast<QComboBox*>(ui->lSkills->cellWidget(i, 1)))
 		{
 			if(widget->currentIndex() > 0)
-				quest.secondary[VLC->skills()->getByIndex(i)->getId()] = widget->currentIndex();
+				quest.mission.secondary[VLC->skills()->getByIndex(i)->getId()] = widget->currentIndex();
 		}
 	}
 	
-	quest.creatures.clear();
+	quest.mission.creatures.clear();
 	for(int i = 0; i < ui->lCreatures->rowCount(); ++i)
 	{
 		int index = ui->lCreatures->item(i, 0)->data(Qt::UserRole).toInt();
 		if(auto * widget = qobject_cast<QSpinBox*>(ui->lCreatures->cellWidget(i, 1)))
 			if(widget->value())
-				quest.creatures.emplace_back(VLC->creatures()->getByIndex(index)->getId(), widget->value());
+				quest.mission.creatures.emplace_back(VLC->creatures()->getByIndex(index)->getId(), widget->value());
 	}
 	
-	quest.heroes.clear();
+	quest.mission.heroes.clear();
 	for(int i = 0; i < ui->lHeroes->count(); ++i)
 	{
 		if(ui->lHeroes->item(i)->checkState() == Qt::Checked)
-			quest.heroes.emplace_back(ui->lHeroes->item(i)->data(Qt::UserRole).toInt());
+			quest.mission.heroes.emplace_back(ui->lHeroes->item(i)->data(Qt::UserRole).toInt());
 	}
 	
-	quest.heroClasses.clear();
+	quest.mission.heroClasses.clear();
 	for(int i = 0; i < ui->lHeroClasses->count(); ++i)
 	{
 		if(ui->lHeroClasses->item(i)->checkState() == Qt::Checked)
-			quest.heroClasses.emplace_back(ui->lHeroClasses->item(i)->data(Qt::UserRole).toInt());
+			quest.mission.heroClasses.emplace_back(ui->lHeroClasses->item(i)->data(Qt::UserRole).toInt());
 	}
 	
-	quest.players.clear();
+	quest.mission.players.clear();
 	for(int i = 0; i < ui->lPlayers->count(); ++i)
 	{
 		if(ui->lPlayers->item(i)->checkState() == Qt::Checked)
-			quest.players.emplace_back(ui->lPlayers->item(i)->data(Qt::UserRole).toInt());
+			quest.mission.players.emplace_back(ui->lPlayers->item(i)->data(Qt::UserRole).toInt());
 	}
 	
 	//quest.killTarget is set directly in object picking
@@ -358,7 +358,7 @@ void QuestWidget::onTargetPicked(const CGObjectInstance * obj)
 	
 	if(!obj) //discarded
 	{
-		quest.killTarget = -1;
+		quest.killTarget = ObjectInstanceID::NONE;
 		ui->lKillTarget->setText("");
 		return;
 	}