Further refactoring

lib/CGameState.cpp

@@ -1266,7 +1266,7 @@ void CGameState::prepareCrossoverHeroes(std::vector<CGameState::CampaignHeroRepl
 
 	// TODO replace magic numbers with named constants
 	// TODO this logic (what should be kept) should be part of CScenarioTravel and be exposed via some clean set of methods
-	if(!(travelOptions.whatHeroKeeps & 1))
+	if(!travelOptions.whatHeroKeeps.experience)
 	{
 		//trimming experience
 		for(CGHeroInstance * cgh : crossoverHeroes)
@@ -1275,7 +1275,7 @@ void CGameState::prepareCrossoverHeroes(std::vector<CGameState::CampaignHeroRepl
 		}
 	}
 
-	if(!(travelOptions.whatHeroKeeps & 2))
+	if(!travelOptions.whatHeroKeeps.primarySkills)
 	{
 		//trimming prim skills
 		for(CGHeroInstance * cgh : crossoverHeroes)
@@ -1291,7 +1291,7 @@ void CGameState::prepareCrossoverHeroes(std::vector<CGameState::CampaignHeroRepl
 		}
 	}
 
-	if(!(travelOptions.whatHeroKeeps & 4))
+	if(!travelOptions.whatHeroKeeps.secondarySkills)
 	{
 		//trimming sec skills
 		for(CGHeroInstance * cgh : crossoverHeroes)
@@ -1301,7 +1301,7 @@ void CGameState::prepareCrossoverHeroes(std::vector<CGameState::CampaignHeroRepl
 		}
 	}
 
-	if(!(travelOptions.whatHeroKeeps & 8))
+	if(!travelOptions.whatHeroKeeps.spells)
 	{
 		for(CGHeroInstance * cgh : crossoverHeroes)
 		{
@@ -1309,7 +1309,7 @@ void CGameState::prepareCrossoverHeroes(std::vector<CGameState::CampaignHeroRepl
 		}
 	}
 
-	if(!(travelOptions.whatHeroKeeps & 16))
+	if(!travelOptions.whatHeroKeeps.artifacts)
 	{
 		//trimming artifacts
 		for(CGHeroInstance * hero : crossoverHeroes)
@@ -1329,9 +1329,7 @@ void CGameState::prepareCrossoverHeroes(std::vector<CGameState::CampaignHeroRepl
 				if(!art)
 					continue;
 
-				int id  = art->artType->getId();
-				assert( 8*18 > id );//number of arts that fits into h3m format
-				bool takeable = travelOptions.artifsKeptByHero[id / 8] & ( 1 << (id%8) );
+				bool takeable = travelOptions.artifactsKeptByHero.count(art->artType->getId());
 
 				ArtifactLocation al(hero, artifactPosition);
 				if(!takeable  &&  !al.getSlot()->locked)  //don't try removing locked artifacts -> it crashes #1719
@@ -1346,7 +1344,7 @@ void CGameState::prepareCrossoverHeroes(std::vector<CGameState::CampaignHeroRepl
 		auto shouldSlotBeErased = [&](const std::pair<SlotID, CStackInstance *> & j) -> bool
 		{
 			CreatureID::ECreatureID crid = j.second->getCreatureID().toEnum();
-			return !(travelOptions.monstersKeptByHero[crid / 8] & (1 << (crid % 8)));
+			return !travelOptions.monstersKeptByHero.count(crid);
 		};
 
 		auto stacksCopy = cgh->stacks; //copy of the map, so we can iterate iover it and remove stacks

lib/mapping/CCampaignHandler.cpp

@@ -282,13 +282,6 @@ CScenarioTravel CCampaignHandler::readScenarioTravelFromJson(JsonNode & reader)
 {
 	CScenarioTravel ret;
 
-	std::map<std::string, ui8> heroKeepsMap = {
-		{"experience", 1},
-		{"primarySkill", 2},
-		{"secondarySkill", 4},
-		{"spells", 8},
-		{"artifacts", 16}
-	};
 	std::map<std::string, ui8> startOptionsMap = {
 		{"none", 0},
 		{"bonus", 1},
@@ -337,31 +330,25 @@ CScenarioTravel CCampaignHandler::readScenarioTravelFromJson(JsonNode & reader)
 	};
 	
 	for(auto & k : reader["heroKeeps"].Vector())
-		ret.whatHeroKeeps |= heroKeepsMap[k.String()];
+	{
+		if(k.String() == "experience") ret.whatHeroKeeps.experience = true;
+		if(k.String() == "primarySkills") ret.whatHeroKeeps.primarySkills = true;
+		if(k.String() == "secondarySkills") ret.whatHeroKeeps.secondarySkills = true;
+		if(k.String() == "spells") ret.whatHeroKeeps.spells = true;
+		if(k.String() == "artifacts") ret.whatHeroKeeps.artifacts = true;
+	}
 	
 	for(auto & k : reader["keepCreatures"].Vector())
 	{
 		if(auto identifier = VLC->modh->identifiers.getIdentifier(CModHandler::scopeMap(), "creature", k.String()))
-		{
-			int creId = identifier.get();
-			if(creId >= ret.monstersKeptByHero.size())
-				logGlobal->warn("VCMP Loading: creature %s with id %d isn't supported yet", k.String(), creId);
-			else
-				ret.monstersKeptByHero[creId / 8] |= (1 << creId % 8);
-		}
+			ret.monstersKeptByHero.insert(CreatureID(identifier.get()));
 		else
 			logGlobal->warn("VCMP Loading: keepCreatures contains unresolved identifier %s", k.String());
 	}
 	for(auto & k : reader["keepArtifacts"].Vector())
 	{
 		if(auto identifier = VLC->modh->identifiers.getIdentifier(CModHandler::scopeMap(), "artifact", k.String()))
-		{
-			int artId = identifier.get();
-			if(artId >= ret.artifsKeptByHero.size())
-				logGlobal->warn("VCMP Loading: artifact %s with id %d isn't supported yet", k.String(), artId);
-			else
-				ret.artifsKeptByHero[artId / 8] |= (1 << artId % 8);
-		}
+			ret.artifactsKeptByHero.insert(ArtifactID(identifier.get()));
 		else
 			logGlobal->warn("VCMP Loading: keepArtifacts contains unresolved identifier %s", k.String());
 	}
@@ -491,8 +478,8 @@ CCampaignHeader CCampaignHandler::readHeaderFromMemory( CBinaryReader & reader,
 	if (ret.version > CampaignVersion::RoE)
 		ret.difficultyChoosenByPlayer = reader.readInt8();
 	else
-		ret.difficultyChoosenByPlayer = 0;
-	ret.music = reader.readInt8();
+		ret.difficultyChoosenByPlayer = false;
+	reader.readInt8(); //music - skip as unused
 	ret.filename = filename;
 	ret.modName = modName;
 	ret.encoding = encoding;
@@ -518,7 +505,7 @@ CCampaignScenario CCampaignHandler::readScenarioFromMemory( CBinaryReader & read
 	CCampaignScenario ret;
 	ret.conquered = false;
 	ret.mapName = reader.readBaseString();
-	ret.packedMapSize = reader.readUInt32();
+	reader.readUInt32(); //packedMapSize - not used
 	if(header.numberOfScenarios > 8) //unholy alliance
 	{
 		ret.loadPreconditionRegions(reader.readUInt16());
@@ -551,18 +538,29 @@ CScenarioTravel CCampaignHandler::readScenarioTravelFromMemory(CBinaryReader & r
 {
 	CScenarioTravel ret;
 
-	ret.whatHeroKeeps = reader.readUInt8();
-	reader.getStream()->read(ret.monstersKeptByHero.data(), ret.monstersKeptByHero.size());
-
-	if (version < CampaignVersion::SoD)
+	ui8 whatHeroKeeps = reader.readUInt8();
+	ret.whatHeroKeeps.experience = whatHeroKeeps & 1;
+	ret.whatHeroKeeps.primarySkills = whatHeroKeeps & 2;
+	ret.whatHeroKeeps.secondarySkills = whatHeroKeeps & 4;
+	ret.whatHeroKeeps.spells = whatHeroKeeps & 8;
+	ret.whatHeroKeeps.artifacts = whatHeroKeeps & 16;
+		
+	auto bitMaskToId = [&reader]<typename T>(std::set<T> & container, int size)
 	{
-		ret.artifsKeptByHero.fill(0);
-		reader.getStream()->read(ret.artifsKeptByHero.data(), ret.artifsKeptByHero.size() - 1);
-	}
+		for(int iId = 0, byte = 0; iId < size * 8; ++iId)
+		{
+			if(iId % 8 == 0)
+				byte = reader.readUInt8();
+			if(byte & (1 << iId % 8))
+				container.insert(T(iId));
+		}
+	};
+	
+	bitMaskToId(ret.monstersKeptByHero, 19);
+	if(version < CampaignVersion::SoD)
+		bitMaskToId(ret.artifactsKeptByHero, 17);
 	else
-	{
-		reader.getStream()->read(ret.artifsKeptByHero.data(), ret.artifsKeptByHero.size());
-	}
+		bitMaskToId(ret.artifactsKeptByHero, 18);
 
 	ret.startOptions = reader.readUInt8();
 

lib/mapping/CCampaignHandler.h

@@ -77,8 +77,7 @@ public:
 	CampaignRegions campaignRegions;
 	int numberOfScenarios = 0;
 	std::string name, description;
-	ui8 difficultyChoosenByPlayer = 0;
-	ui8 music = 0; //CmpMusic.txt, start from 0, field is unused in vcmi
+	bool difficultyChoosenByPlayer = false;
 	bool valid = false;
 
 	std::string filename;
@@ -90,34 +89,44 @@ public:
 	template <typename Handler> void serialize(Handler &h, const int formatVersion)
 	{
 		h & version;
-		if(!h.saving && formatVersion < 821)
-		{
-			ui8 campId = 0; //legacy field
-			h & campId;
-			loadLegacyData(campId);
-		}
-		else
-		{
-			h & campaignRegions;
-			h & numberOfScenarios;
-		}
+		h & campaignRegions;
+		h & numberOfScenarios;
 		h & name;
 		h & description;
 		h & difficultyChoosenByPlayer;
-		if(formatVersion < 821)
-			h & music; //deprecated
 		h & filename;
 		h & modName;
 		h & encoding;
+		h & valid;
 	}
 };
 
 class DLL_LINKAGE CScenarioTravel
 {
 public:
-	ui8 whatHeroKeeps = 0; //bitfield [0] - experience, [1] - prim skills, [2] - sec skills, [3] - spells, [4] - artifacts
-	std::array<ui8, 19> monstersKeptByHero;
-	std::array<ui8, 18> artifsKeptByHero;
+	
+	struct DLL_LINKAGE WhatHeroKeeps
+	{
+		bool experience = false;
+		bool primarySkills = false;
+		bool secondarySkills = false;
+		bool spells = false;
+		bool artifacts = false;
+		
+		template <typename Handler> void serialize(Handler &h, const int formatVersion)
+		{
+			h & experience;
+			h & primarySkills;
+			h & secondarySkills;
+			h & spells;
+			h & artifacts;
+		}
+	};
+	
+	WhatHeroKeeps whatHeroKeeps;
+	
+	std::set<CreatureID> monstersKeptByHero;
+	std::set<ArtifactID> artifactsKeptByHero;
 
 	ui8 startOptions = 0; //1 - start bonus, 2 - traveling hero, 3 - hero options
 
@@ -147,7 +156,7 @@ public:
 	{
 		h & whatHeroKeeps;
 		h & monstersKeptByHero;
-		h & artifsKeptByHero;
+		h & artifactsKeptByHero;
 		h & startOptions;
 		h & playerColor;
 		h & bonusesToChoose;
@@ -176,7 +185,6 @@ public:
 
 	std::string mapName; //*.h3m
 	std::string scenarioName; //from header. human-readble
-	ui32 packedMapSize = 0; //generally not used
 	std::set<ui8> preconditionRegions; //what we need to conquer to conquer this one (stored as bitfield in h3c)
 	ui8 regionColor = 0;
 	ui8 difficulty = 0;
@@ -200,7 +208,6 @@ public:
 	{
 		h & mapName;
 		h & scenarioName;
-		h & packedMapSize;
 		h & preconditionRegions;
 		h & regionColor;
 		h & difficulty;