Cleanup and formatting of H3M loader code

lib/mapObjects/MiscObjects.cpp

@@ -1394,11 +1394,16 @@ void CGArtifact::serializeJsonOptions(JsonSerializeFormat& handler)
 
 void CGWitchHut::initObj(CRandomGenerator & rand)
 {
-	if (allowedAbilities.empty()) //this can happen for RMG. regular maps load abilities from map file
+	if (allowedAbilities.empty()) //this can happen for RMG and RoE maps.
 	{
-		// Necromancy can't be learned on random maps
-		for(int i = 0; i < VLC->skillh->size(); i++)
-			if(VLC->skillh->getByIndex(i)->getId() != SecondarySkill::NECROMANCY)
+		auto defaultAllowed = VLC->skillh->getDefaultAllowed();
+
+		// Necromancy and Leadership can't be learned by default
+		defaultAllowed[SecondarySkill::NECROMANCY] = false;
+		defaultAllowed[SecondarySkill::LEADERSHIP] = false;
+
+		for(int i = 0; i < defaultAllowed.size(); i++)
+			if (defaultAllowed[i])
 				allowedAbilities.insert(i);
 	}
 	ability = *RandomGeneratorUtil::nextItem(allowedAbilities, rand);

lib/mapping/MapFeaturesH3M.cpp

@@ -10,19 +10,25 @@
 
 #include "StdInc.h"
 #include "MapFeaturesH3M.h"
+
 #include "CMap.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
 MapFormatFeaturesH3M MapFormatFeaturesH3M::find(EMapFormat format)
 {
-	switch (format)
+	switch(format)
 	{
-		case EMapFormat::ROE:  return getFeaturesROE();
-		case EMapFormat::AB:   return getFeaturesAB();
-		case EMapFormat::SOD:  return getFeaturesSOD();
-		case EMapFormat::WOG:  return getFeaturesWOG();
-		case EMapFormat::HOTA: return getFeaturesHOTA();
+		case EMapFormat::ROE:
+			return getFeaturesROE();
+		case EMapFormat::AB:
+			return getFeaturesAB();
+		case EMapFormat::SOD:
+			return getFeaturesSOD();
+		case EMapFormat::WOG:
+			return getFeaturesWOG();
+		case EMapFormat::HOTA:
+			return getFeaturesHOTA();
 		default:
 			throw std::runtime_error("Invalid map format!");
 	}

lib/mapping/MapFormatH3M.cpp

@@ -29,7 +29,6 @@
 #include "../TerrainHandler.h"
 #include "../RoadHandler.h"
 #include "../RiverHandler.h"
-#include "../NetPacksBase.h"
 
 #include <boost/crc.hpp>
 
@@ -41,7 +40,7 @@ static std::string convertMapName(std::string input)
 	boost::algorithm::to_lower(input);
 	boost::algorithm::trim(input);
 
-	size_t slashPos = input.find_last_of("/");
+	size_t slashPos = input.find_last_of('/');
 
 	if (slashPos != std::string::npos)
 		return input.substr(slashPos + 1);
@@ -175,7 +174,7 @@ void CMapLoaderH3M::readHeader()
 
 	// Read map name, description, dimensions,...
 	mapHeader->areAnyPlayers = reader->readBool();
-	mapHeader->height = mapHeader->width = reader->readUInt32();
+	mapHeader->height = mapHeader->width = reader->readInt32();
 	mapHeader->twoLevel = reader->readBool();
 	mapHeader->name = readLocalizedString("header.name");
 	mapHeader->description = readLocalizedString("header.description");
@@ -256,8 +255,7 @@ void CMapLoaderH3M::readPlayerInfo()
 		if(features.levelAB)
 		{
 			reader->skipUnused(1); //TODO: check meaning?
-			int heroCount = reader->readUInt8();
-			reader->skipZero(3);
+			uint32_t heroCount = reader->readUInt32();
 			for(int pp = 0; pp < heroCount; ++pp)
 			{
 				SHeroName vv;
@@ -355,7 +353,7 @@ void CMapLoaderH3M::readVictoryLossConditions()
 			{
 				EventCondition cond(EventCondition::HAVE_CREATURES);
 				cond.objectType = reader->readCreature();
-				cond.value = reader->readUInt32();
+				cond.value = reader->readInt32();
 
 				specialVictory.effect.toOtherMessage = VLC->generaltexth->allTexts[277];
 				specialVictory.onFulfill = VLC->generaltexth->allTexts[276];
@@ -366,7 +364,7 @@ void CMapLoaderH3M::readVictoryLossConditions()
 			{
 				EventCondition cond(EventCondition::HAVE_RESOURCES);
 				cond.objectType = reader->readUInt8();
-				cond.value = reader->readUInt32();
+				cond.value = reader->readInt32();
 
 				specialVictory.effect.toOtherMessage = VLC->generaltexth->allTexts[279];
 				specialVictory.onFulfill = VLC->generaltexth->allTexts[278];
@@ -591,14 +589,14 @@ void CMapLoaderH3M::readTeamInfo()
 
 void CMapLoaderH3M::readAllowedHeroes()
 {
-	mapHeader->allowedHeroes.resize(VLC->heroh->size(), true);
+	mapHeader->allowedHeroes = VLC->heroh->getDefaultAllowed();
 
 	reader->readBitmask(mapHeader->allowedHeroes, features.heroesBytes, features.heroesCount, false);
 
 	//TODO: unknown value. Check meaning? Only present in campaign maps.
 	if(features.levelAB)
 	{
-		int placeholdersQty = reader->readUInt32();
+		uint32_t placeholdersQty = reader->readUInt32();
 		reader->skipUnused(placeholdersQty * 1);
 	}
 }
@@ -625,11 +623,11 @@ void CMapLoaderH3M::readDisposedHeroes()
 
 void CMapLoaderH3M::readAllowedArtifacts()
 {
-	map->allowedArtifact.resize (VLC->arth->objects.size(),true); //handle new artifacts, make them allowed by default
+	map->allowedArtifact = VLC->arth->getDefaultAllowed();
 
 	// Reading allowed artifacts:  17 or 18 bytes
 	if(features.levelAB)
-		reader->readBitmask(map->allowedArtifact, features.artifactsBytes, features.artifactsCount);
+		reader->readBitmask(map->allowedArtifact, features.artifactsBytes, features.artifactsCount, true);
 
 	// ban combo artifacts
 	if (!features.levelSOD)
@@ -664,21 +662,13 @@ void CMapLoaderH3M::readAllowedArtifacts()
 
 void CMapLoaderH3M::readAllowedSpellsAbilities()
 {
-	// Read allowed spells, including new ones
-	map->allowedSpell.resize(VLC->spellh->objects.size(), true);
-
-	// Read allowed abilities
-	map->allowedAbilities.resize(VLC->skillh->objects.size(), true);
+	map->allowedSpell = VLC->spellh->getDefaultAllowed();
+	map->allowedAbilities = VLC->skillh->getDefaultAllowed();
 
 	if(features.levelSOD)
 	{
-		// Reading allowed spells (9 bytes)
-		const int spell_bytes = 9;
-		reader->readBitmask(map->allowedSpell, spell_bytes, GameConstants::SPELLS_QUANTITY);
-
-		// Allowed hero's abilities (4 bytes)
-		const int abil_bytes = 4;
-		reader->readBitmask(map->allowedAbilities, abil_bytes, GameConstants::SKILL_QUANTITY);
+		reader->readBitmask(map->allowedSpell, features.spellsBytes, features.spellsCount, true);
+		reader->readBitmask(map->allowedAbilities, features.skillsBytes, features.skillsCount, true);
 	}
 
 	//do not generate special abilities and spells
@@ -728,7 +718,7 @@ void CMapLoaderH3M::readPredefinedHeroes()
 		bool hasSecSkills = reader->readBool();
 		if(hasSecSkills)
 		{
-			int howMany = reader->readUInt32();
+			uint32_t howMany = reader->readUInt32();
 			hero->secSkills.resize(howMany);
 			for(int yy = 0; yy < howMany; ++yy)
 			{
@@ -864,7 +854,7 @@ void CMapLoaderH3M::readTerrain()
 
 void CMapLoaderH3M::readDefInfo()
 {
-	int defAmount = reader->readUInt32();
+	uint32_t defAmount = reader->readUInt32();
 
 	templates.reserve(defAmount);
 
@@ -879,15 +869,15 @@ void CMapLoaderH3M::readDefInfo()
 
 void CMapLoaderH3M::readObjects()
 {
-	int howManyObjs = reader->readUInt32();
+	uint32_t howManyObjs = reader->readUInt32();
 
-	for(int ww = 0; ww < howManyObjs; ++ww)
+	for(uint32_t ww = 0; ww < howManyObjs; ++ww)
 	{
 		CGObjectInstance * nobj = nullptr;
 
 		int3 objPos = reader->readInt3();
 
-		int defnum = reader->readUInt32();
+		uint32_t defnum = reader->readUInt32();
 		ObjectInstanceID idToBeGiven = ObjectInstanceID(static_cast<si32>(map->objects.size()));
 
 		std::shared_ptr<const ObjectTemplate> objTempl = templates.at(defnum);
@@ -903,7 +893,7 @@ void CMapLoaderH3M::readObjects()
 				readMessageAndGuards(evnt->message, evnt, objPos);
 
 				evnt->gainedExp = reader->readUInt32();
-				evnt->manaDiff = reader->readUInt32();
+				evnt->manaDiff = reader->readInt32();
 				evnt->moraleDiff = reader->readInt8();
 				evnt->luckDiff = reader->readInt8();
 
@@ -976,7 +966,7 @@ void CMapLoaderH3M::readObjects()
 				//type will be set during initialization
 				cre->putStack(SlotID(0), hlp);
 
-				cre->character = reader->readUInt8();
+				cre->character = reader->readInt8();
 
 				bool hasMessage = reader->readBool();
 				if(hasMessage)
@@ -1009,21 +999,20 @@ void CMapLoaderH3M::readObjects()
 				auto * wh = new CGWitchHut();
 				nobj = wh;
 
+				// AB and later maps have allowed abilities defined in H3M
 				if(features.levelAB)
 				{
-					reader->readBitmask(wh->allowedAbilities, features.skillsBytes, features.skillsCount);
-					// enable new (modded) skills
+					reader->readBitmask(wh->allowedAbilities, features.skillsBytes, features.skillsCount, false);
+
 					if(wh->allowedAbilities.size() != 1)
 					{
-						for(int skillID = features.skillsCount; skillID < VLC->skillh->size(); ++skillID)
-							wh->allowedAbilities.insert(skillID);
+						auto defaultAllowed = VLC->skillh->getDefaultAllowed();
+
+						for(int skillID = 0; skillID < VLC->skillh->size(); ++skillID)
+							if (defaultAllowed[skillID])
+								wh->allowedAbilities.insert(skillID);
 					}
 				}
-				else
-				{
-					for(int skillID = 0; skillID < VLC->skillh->size(); ++skillID)
-						wh->allowedAbilities.insert(skillID);
-				}
 				break;
 			}
 		case Obj::SCHOLAR:
@@ -1143,7 +1132,7 @@ void CMapLoaderH3M::readObjects()
 				readMessageAndGuards(box->message, box, objPos);
 
 				box->gainedExp = reader->readUInt32();
-				box->manaDiff = reader->readUInt32();
+				box->manaDiff = reader->readInt32();
 				box->moraleDiff = reader->readInt8();
 				box->luckDiff = reader->readInt8();
 
@@ -1177,7 +1166,7 @@ void CMapLoaderH3M::readObjects()
 		case Obj::GRAIL:
 			{
 				map->grailPos = objPos;
-				map->grailRadius = reader->readUInt32();
+				map->grailRadius = reader->readInt32();
 				continue;
 			}
 		case Obj::RANDOM_DWELLING: //same as castle + level range
@@ -1252,7 +1241,7 @@ void CMapLoaderH3M::readObjects()
 		case Obj::SHIPYARD:
 			{
 				nobj = new CGShipyard();
-				nobj->setOwner(PlayerColor(reader->readUInt32()));
+				nobj->setOwner(reader->readPlayer32());
 				break;
 			}
 		case Obj::HERO_PLACEHOLDER: //hero placeholder
@@ -1459,7 +1448,7 @@ CGObjectInstance * CMapLoaderH3M::readHero(const ObjectInstanceID & idToBeGiven,
 			nhi->secSkills.clear();
 		}
 
-		int howMany = reader->readUInt32();
+		uint32_t howMany = reader->readUInt32();
 		nhi->secSkills.resize(howMany);
 		for(int yy = 0; yy < howMany; ++yy)
 		{
@@ -1602,10 +1591,8 @@ CGSeerHut * CMapLoaderH3M::readSeerHut(const int3 & position)
 				hut->rID = reader->readUInt8();
 				hut->rVal = reader->readUInt32();
 
-				// Only the first 3 bytes are used. Skip the 4th.
 				assert(hut->rID < features.resourcesCount);
 				assert((hut->rVal & 0x00ffffff) == hut->rVal);
-				hut->rVal = hut->rVal & 0x00ffffff;
 				break;
 			}
 		case CGSeerHut::PRIMARY_SKILL:
@@ -1781,20 +1768,22 @@ CGTownInstance * CMapLoaderH3M::readTown(int castleID, const int3 & position)
 		std::copy(spellsMask.begin(), spellsMask.end(), std::back_inserter(nt->obligatorySpells));
 	}
 
-	if (features.levelROE)
 	{
 		std::set<SpellID> spellsMask;
 
-		reader->readBitmask(spellsMask, features.spellsBytes, features.spellsCount, false );
+		reader->readBitmask(spellsMask, features.spellsBytes, features.spellsCount, true );
 		std::copy(spellsMask.begin(), spellsMask.end(), std::back_inserter(nt->possibleSpells));
-	}
 
-	//add all spells from mods
-	for (int i = SpellID::AFTER_LAST; i < VLC->spellh->objects.size(); ++i)
-		nt->possibleSpells.emplace_back(i);
+		auto defaultAllowed = VLC->spellh->getDefaultAllowed();
+
+		//add all spells from mods
+		for (int i = features.spellsCount; i < defaultAllowed.size(); ++i)
+			if (defaultAllowed[i])
+			nt->possibleSpells.emplace_back(i);
+	}
 
 	// Read castle events
-	int numberOfEvent = reader->readUInt32();
+	uint32_t numberOfEvent = reader->readUInt32();
 
 	for(int gh = 0; gh < numberOfEvent; ++gh)
 	{
@@ -1903,7 +1892,7 @@ std::set<BuildingID> CMapLoaderH3M::convertBuildings(const std::set<BuildingID>
 
 void CMapLoaderH3M::readEvents()
 {
-	int numberOfEvents = reader->readUInt32();
+	uint32_t numberOfEvents = reader->readUInt32();
 	for(int yyoo = 0; yyoo < numberOfEvents; ++yyoo)
 	{
 		CMapEvent ne;

lib/mapping/MapReaderH3M.cpp

@@ -10,6 +10,7 @@
 
 #include "StdInc.h"
 #include "MapReaderH3M.h"
+
 #include "CMap.h"
 #include "../filesystem/CBinaryReader.h"
 
@@ -29,12 +30,12 @@ ArtifactID MapReaderH3M::readArtifact()
 {
 	ArtifactID result;
 
-	if (features.levelAB)
+	if(features.levelAB)
 		result = ArtifactID(reader->readUInt16());
 	else
 		result = ArtifactID(reader->readUInt8());
 
-	if (result == features.artifactIdentifierInvalid)
+	if(result == features.artifactIdentifierInvalid)
 		return ArtifactID::NONE;
 
 	assert(result < features.artifactsCount);
@@ -43,9 +44,9 @@ ArtifactID MapReaderH3M::readArtifact()
 
 HeroTypeID MapReaderH3M::readHero()
 {
-	HeroTypeID result (reader->readUInt8());
+	HeroTypeID result(reader->readUInt8());
 
-	if (result.getNum() == features.heroIdentifierInvalid)
+	if(result.getNum() == features.heroIdentifierInvalid)
 		return HeroTypeID(-1);
 
 	assert(result.getNum() < features.heroesPortraitsCount);
@@ -56,105 +57,103 @@ CreatureID MapReaderH3M::readCreature()
 {
 	CreatureID result;
 
-	if (features.levelAB)
+	if(features.levelAB)
 		result = CreatureID(reader->readUInt16());
 	else
 		result = CreatureID(reader->readUInt8());
 
-	if (result == features.creatureIdentifierInvalid)
+	if(result == features.creatureIdentifierInvalid)
 		return CreatureID::NONE;
 
 	if(result > features.creaturesCount)
 	{
 		// this may be random creature in army/town, to be randomized later
-		CreatureID randomIndex (result.getNum() - features.creatureIdentifierInvalid - 1);
+		CreatureID randomIndex(result.getNum() - features.creatureIdentifierInvalid - 1);
 		assert(randomIndex < CreatureID::NONE);
 		assert(randomIndex > -16);
 		return randomIndex;
 	}
 
 	return result;
-
-
 }
 
 TerrainId MapReaderH3M::readTerrain()
 {
 	TerrainId result(readUInt8());
-	assert (result.getNum() < features.terrainsCount);
+	assert(result.getNum() < features.terrainsCount);
 	return result;
 }
 
 RoadId MapReaderH3M::readRoad()
 {
-	RoadId result(readUInt8());
-	assert (result < Road::ORIGINAL_ROAD_COUNT);
+	RoadId result(readInt8());
+	assert(result < Road::ORIGINAL_ROAD_COUNT);
 	return result;
 }
 
 RiverId MapReaderH3M::readRiver()
 {
-	RiverId result(readUInt8());
-	assert (result < River::ORIGINAL_RIVER_COUNT);
+	RiverId result(readInt8());
+	assert(result < River::ORIGINAL_RIVER_COUNT);
 	return result;
-
 }
 
 SecondarySkill MapReaderH3M::readSkill()
 {
 	SecondarySkill result(readUInt8());
-	assert (result < features.skillsCount);
+	assert(result < features.skillsCount);
 	return result;
 }
 
 SpellID MapReaderH3M::readSpell()
 {
 	SpellID result(readUInt8());
-	if (result == features.spellIdentifierInvalid)
+	if(result == features.spellIdentifierInvalid)
 		return SpellID::NONE;
-	if (result == features.spellIdentifierInvalid - 1)
+	if(result == features.spellIdentifierInvalid - 1)
 		return SpellID::PRESET;
 
-	assert (result < features.spellsCount);
+	assert(result < features.spellsCount);
 	return result;
 }
 
 SpellID MapReaderH3M::readSpell32()
 {
-	SpellID result(readUInt32());
-	if (result == features.spellIdentifierInvalid)
+	SpellID result(readInt32());
+	if(result == features.spellIdentifierInvalid)
 		return SpellID::NONE;
-	assert (result < features.spellsCount);
+	assert(result < features.spellsCount);
 	return result;
 }
 
 PlayerColor MapReaderH3M::readPlayer()
 {
 	PlayerColor result(readUInt8());
-	assert (result < PlayerColor::PLAYER_LIMIT || result == PlayerColor::NEUTRAL);
+	assert(result < PlayerColor::PLAYER_LIMIT || result == PlayerColor::NEUTRAL);
 	return result;
 }
 
 PlayerColor MapReaderH3M::readPlayer32()
 {
-	PlayerColor result(readUInt32());
+	PlayerColor result(readInt32());
 
-	assert (result < PlayerColor::PLAYER_LIMIT || result == PlayerColor::NEUTRAL);
+	assert(result < PlayerColor::PLAYER_LIMIT || result == PlayerColor::NEUTRAL);
 	return result;
 }
 
-void MapReaderH3M::readBitmask(std::vector<bool> & dest, const int byteCount, const int limit, bool negate)
+void MapReaderH3M::readBitmask(std::vector<bool> & dest, const int bytesToRead, const int objectsToRead, bool invert)
 {
-	for(int byte = 0; byte < byteCount; ++byte)
+	for(int byte = 0; byte < bytesToRead; ++byte)
 	{
 		const ui8 mask = reader->readUInt8();
 		for(int bit = 0; bit < 8; ++bit)
 		{
-			if(byte * 8 + bit < limit)
+			if(byte * 8 + bit < objectsToRead)
 			{
+				const size_t index = byte * 8 + bit;
 				const bool flag = mask & (1 << bit);
-				if((negate && flag) || (!negate && !flag)) // FIXME: check PR388
-					dest[byte * 8 + bit] = false;
+				const bool result = (flag != invert);
+				dest[index] = result;
 			}
 		}
 	}
@@ -179,7 +178,7 @@ void MapReaderH3M::skipZero(size_t amount)
 #ifdef NDEBUG
 	skipUnused(amount);
 #else
-	for (size_t i = 0; i < amount; ++i)
+	for(size_t i = 0; i < amount; ++i)
 	{
 		uint8_t value = reader->readUInt8();
 		assert(value == 0);
@@ -187,10 +186,10 @@ void MapReaderH3M::skipZero(size_t amount)
 #endif
 }
 
-void MapReaderH3M::readResourses(TResources& resources)
+void MapReaderH3M::readResourses(TResources & resources)
 {
 	for(int x = 0; x < features.resourcesCount; ++x)
-		resources[x] = reader->readUInt32();
+		resources[x] = reader->readInt32();
 }
 
 bool MapReaderH3M::readBool()
@@ -201,12 +200,12 @@ bool MapReaderH3M::readBool()
 	return result != 0;
 }
 
-ui8  MapReaderH3M::readUInt8()
+ui8 MapReaderH3M::readUInt8()
 {
 	return reader->readUInt8();
 }
 
-si8  MapReaderH3M::readInt8()
+si8 MapReaderH3M::readInt8()
 {
 	return reader->readInt8();
 }

lib/mapping/MapReaderH3M.h

@@ -10,8 +10,8 @@
 
 #pragma once
 
-#include "../ResourceSet.h"
 #include "../GameConstants.h"
+#include "../ResourceSet.h"
 #include "MapFeaturesH3M.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
@@ -41,15 +41,15 @@ public:
 	PlayerColor readPlayer();
 	PlayerColor readPlayer32();
 
-	template <class Identifier>
-	void readBitmask(std::set<Identifier> &dest, const int byteCount, const int limit, bool negate = true)
+	template<class Identifier>
+	void readBitmask(std::set<Identifier> & dest, int bytesToRead, int objectsToRead, bool invert)
 	{
-		std::vector<bool> temp;
-		temp.resize(limit,true);
-		readBitmask(temp, byteCount, limit, negate);
+		std::vector<bool> bitmap;
+		bitmap.resize(objectsToRead, false);
+		readBitmask(bitmap, bytesToRead, objectsToRead, invert);
 
-		for(int i = 0; i< std::min(temp.size(), static_cast<size_t>(limit)); i++)
-			if(temp[i])
+		for(int i = 0; i < bitmap.size(); i++)
+			if(bitmap[i])
 				dest.insert(static_cast<Identifier>(i));
 	}
 
@@ -59,7 +59,7 @@ public:
 	* @param limit max count of vector elements to alter
 	* @param negate if true then set bit in mask means clear flag in vertor
 	*/
-	void readBitmask(std::vector<bool> & dest, int byteCount, int limit, bool negate = true);
+	void readBitmask(std::vector<bool> & dest, int bytesToRead, int objectsToRead, bool invert);
 
 	/**
 	* Helper to read map position
@@ -69,12 +69,12 @@ public:
 	void skipUnused(size_t amount);
 	void skipZero(size_t amount);
 
-	void readResourses(TResources& resources);
+	void readResourses(TResources & resources);
 
 	bool readBool();
 
-	ui8  readUInt8();
-	si8  readInt8();
+	ui8 readUInt8();
+	si8 readInt8();
 	ui16 readUInt16();
 	si16 readInt16();
 	ui32 readUInt32();
@@ -83,6 +83,7 @@ public:
 	std::string readBaseString();
 
 	CBinaryReader & getInternalReader();
+
 private:
 	MapFormatFeaturesH3M features;