code review (first batch)

client/lobby/CBonusSelection.cpp

@@ -271,13 +271,13 @@ void CBonusSelection::createBonusesIcons()
 
 			switch(bonDescs[i].info1)
 			{
-				case 0xFD: //wood + ore
+				case EGameResID::COMMON: //wood + ore
 				{
 					desc.replaceLocalString(EMetaText::GENERAL_TXT, 721);
 					picNumber = 7;
 					break;
 				}
-				case 0xFE: //wood + ore
+				case EGameResID::RARE : //mercury + sulfur + crystal + gems
 				{
 					desc.replaceLocalString(EMetaText::GENERAL_TXT, 722);
 					picNumber = 8;
@@ -305,7 +305,7 @@ void CBonusSelection::createBonusesIcons()
 		}
 
 		case CampaignBonusType::HERO:
-			if(bonDescs[i].info2 == 0xFFFF)
+			if(bonDescs[i].info2 == HeroTypeID::RANDOM)
 			{
 				desc.appendLocalString(EMetaText::GENERAL_TXT, 720); // Start with random hero
 				picNumber = -1;

lib/campaign/CampaignHandler.cpp

@@ -281,23 +281,21 @@ static const std::map<std::string, ui32> primarySkillsMap = {
 };
 
 static const std::map<std::string, ui16> heroSpecialMap = {
-	{"strongest", 0xFFFD},
+	{"strongest", HeroTypeID::STRONGEST},
-	{"generated", 0xFFFE},
+	{"generated", HeroTypeID::GENERATED},
-	{"random", 0xFFFF}
+	{"random", HeroTypeID::RANDOM}
 };
 
 static const std::map<std::string, ui8> resourceTypeMap = {
-	//FD - wood+ore
+	{"wood", EGameResID::WOOD},
-	//FE - mercury+sulfur+crystal+gem
+	{"mercury", EGameResID::MERCURY},
-	{"wood", 0},
+	{"ore", EGameResID::ORE},
-	{"mercury", 1},
+	{"sulfur", EGameResID::SULFUR},
-	{"ore", 2},
+	{"crystal", EGameResID::CRYSTAL},
-	{"sulfur", 3},
+	{"gems", EGameResID::GEMS},
-	{"crystal", 4},
+	{"gold", EGameResID::GOLD},
-	{"gems", 5},
+	{"common", EGameResID::COMMON},
-	{"gold", 6},
+	{"rare", EGameResID::RARE}
-	{"common", 0xFD},
-	{"rare", 0xFE}
 };
 
 CampaignTravel CampaignHandler::readScenarioTravelFromJson(JsonNode & reader)

lib/constants/EntityIdentifiers.cpp

@@ -53,9 +53,11 @@ const BattleID BattleID::NONE(-1);
 const QueryID QueryID::NONE(-1);
 const QueryID QueryID::CLIENT(-2);
 const HeroTypeID HeroTypeID::NONE(-1);
-const HeroTypeID HeroTypeID::RANDOM(-2);
 const HeroTypeID HeroTypeID::GEM(27);
 const HeroTypeID HeroTypeID::SOLMYR(45);
+const HeroTypeID HeroTypeID::STRONGEST(0xFFFD);
+const HeroTypeID HeroTypeID::GENERATED(0xFFFE);
+const HeroTypeID HeroTypeID::RANDOM(0xFFFF);
 
 const ObjectInstanceID ObjectInstanceID::NONE(-1);
 

lib/constants/EntityIdentifiers.h

@@ -102,10 +102,13 @@ public:
 	const HeroType * toEntity(const Services * services) const;
 
 	static const HeroTypeID NONE;
-	static const HeroTypeID RANDOM;
 	static const HeroTypeID GEM; // aka Gem, Sorceress in campaign
 	static const HeroTypeID SOLMYR; // aka Young Yog in campaigns
 
+	static const HeroTypeID STRONGEST;
+	static const HeroTypeID GENERATED;
+	static const HeroTypeID RANDOM;
+
 	bool isValid() const
 	{
 		return getNum() >= 0;
@@ -1043,6 +1046,8 @@ public:
 		COUNT,
 
 		WOOD_AND_ORE = 127,  // special case for town bonus resource
+		COMMON = 0xFD, // campaign bonus
+		RARE = 0xFE, // campaign bonus
 		NONE = -1
 	};
 };

lib/gameState/CGameStateCampaign.cpp

@@ -209,7 +209,7 @@ void CGameStateCampaign::placeCampaignHeroes()
 		if(it != gameState->scenarioOps->playerInfos.end())
 		{
 			HeroTypeID heroTypeId = HeroTypeID(campaignBonus->info2);
-			if(heroTypeId.getNum() == 0xffff) // random bonus hero
+			if(heroTypeId.getNum() == HeroTypeID::RANDOM) // random bonus hero
 			{
 				heroTypeId = gameState->pickUnusedHeroTypeRandomly(playerColor);
 			}
@@ -529,7 +529,7 @@ void CGameStateCampaign::generateCampaignHeroesToReplace()
 void CGameStateCampaign::initHeroes()
 {
 	auto chosenBonus = currentBonus();
-	if (chosenBonus && chosenBonus->isBonusForHero() && chosenBonus->info1 != 0xFFFE) //exclude generated heroes
+	if (chosenBonus && chosenBonus->isBonusForHero() && chosenBonus->info1 != HeroTypeID::GENERATED) //exclude generated heroes
 	{
 		//find human player
 		PlayerColor humanPlayer=PlayerColor::NEUTRAL;
@@ -545,7 +545,7 @@ void CGameStateCampaign::initHeroes()
 
 		const auto & heroes = gameState->players[humanPlayer].getHeroes();
 
-		if (chosenBonus->info1 == 0xFFFD) //most powerful
+		if (chosenBonus->info1 == HeroTypeID::STRONGEST) //most powerful
 		{
 			int maxB = -1;
 			for (int b=0; b<heroes.size(); ++b)
@@ -610,11 +610,11 @@ void CGameStateCampaign::initStartingResources()
 				case 0: case 1: case 2: case 3: case 4: case 5: case 6:
 					res.push_back(chosenBonus->info1);
 					break;
-				case 0xFD: //wood+ore
+				case EGameResID::COMMON: //wood+ore
 					res.push_back(GameResID(EGameResID::WOOD));
 					res.push_back(GameResID(EGameResID::ORE));
 					break;
-				case 0xFE:  //rare
+				case EGameResID::RARE:  //rare
 					res.push_back(GameResID(EGameResID::MERCURY));
 					res.push_back(GameResID(EGameResID::SULFUR));
 					res.push_back(GameResID(EGameResID::CRYSTAL));

mapeditor/BitmapHandler.cpp

@@ -69,7 +69,7 @@ namespace BitmapHandler
 			it = (int)size - 256 * 3;
 			for(int i = 0; i < 256; i++)
 			{
-				unsigned char bytes[4];
+				std::array<uint8_t, 4> bytes;
 				bytes[0] = pcx[it++];
 				bytes[1] = pcx[it++];
 				bytes[2] = pcx[it++];

mapeditor/StdInc.h

@@ -11,9 +11,6 @@
 
 #include "../Global.h"
 
-#define VCMI_EDITOR_NAME "VCMI Map Editor"
-#define VCMI_CAMP_EDITOR_NAME "VCMI Campaign Editor"
-
 #include <QtWidgets>
 #include <QStringList>
 #include <QSet>

mapeditor/campaigneditor/campaigneditor.cpp

@@ -124,7 +124,7 @@ bool CampaignEditor::getAnswerAboutUnsavedChanges()
 void CampaignEditor::setTitle()
 {
 	QFileInfo fileInfo(filename);
-	QString title = QString("%1%2 - %3 (%4)").arg(fileInfo.fileName(), unsaved ? "*" : "", VCMI_CAMP_EDITOR_NAME, GameConstants::VCMI_VERSION.c_str());
+	QString title = QString("%1%2 - %3 (%4)").arg(fileInfo.fileName(), unsaved ? "*" : "", tr("VCMI Campaign Editor"), GameConstants::VCMI_VERSION.c_str());
 	setWindowTitle(title);
 }
 
@@ -138,8 +138,9 @@ bool CampaignEditor::saveCampaign()
 {
 	if(campaignState->mapPieces.size() != campaignState->campaignRegions.regions.size())
 	{
-		QMessageBox::critical(nullptr, tr("Maps missing"), tr("Not all Regions have a map. Please add them in Scenario Properties."));
+		auto reply = QMessageBox::question(nullptr, tr("Maps missing"), tr("Not all regions have a map. Do you want to continue?"), QMessageBox::Yes|QMessageBox::No);
-		return false;
+		if (reply != QMessageBox::Yes)
+			return false;
 	}
 
 	Helper::saveCampaign(campaignState, filename);

mapeditor/campaigneditor/campaignproperties.cpp

@@ -15,6 +15,7 @@
 #include "../../lib/texts/CGeneralTextHandler.h"
 #include "../../lib/campaign/CampaignState.h"
 #include "../../lib/constants/StringConstants.h"
+#include "../../lib/json/JsonNode.h"
 
 CampaignProperties::CampaignProperties(std::shared_ptr<CampaignState> campaignState):
 	ui(new Ui::CampaignProperties),
@@ -36,9 +37,12 @@ CampaignProperties::CampaignProperties(std::shared_ptr<CampaignState> campaignSt
 	ui->lineEditMusic->setText(QString::fromStdString(campaignState->music.getName()));
 	ui->checkBoxScenarioDifficulty->setChecked(campaignState->difficultyChosenByPlayer);
 	
-	for (int i = 0; i < 20; i++)
+	const JsonNode legacyRegionConfig(JsonPath::builtin("config/campaign_regions.json"));
+	int legacyRegionNumber = legacyRegionConfig["campaign_regions"].Vector().size();
+
+	for (int i = 0; i < legacyRegionNumber; i++)
 		ui->comboBoxRegionPreset->insertItem(i, QString::fromStdString(LIBRARY->generaltexth->translate("core.camptext.names", i)));
-	ui->comboBoxRegionPreset->insertItem(20, tr("Custom"));
+	ui->comboBoxRegionPreset->insertItem(legacyRegionNumber, tr("Custom"));
 	ui->comboBoxRegionPreset->setCurrentIndex(20);
 
 	loadRegion();

mapeditor/campaigneditor/scenarioproperties.cpp

@@ -112,9 +112,9 @@ ScenarioProperties::ScenarioProperties(std::shared_ptr<CampaignState> campaignSt
 	{
 		heroSelection.emplace(hero->getId().getNum(), hero->getNameTranslated());
 	});
-	heroSelection.emplace(0xFFFD, tr("Strongest").toStdString());
+	heroSelection.emplace(HeroTypeID::STRONGEST, tr("Strongest").toStdString());
-	heroSelection.emplace(0xFFFE, tr("Generated").toStdString());
+	heroSelection.emplace(HeroTypeID::GENERATED, tr("Generated").toStdString());
-	heroSelection.emplace(0xFFFF, tr("Random").toStdString());
+	heroSelection.emplace(HeroTypeID::RANDOM, tr("Random").toStdString());
 
 	reloadMapRelatedUi();
 

mapeditor/campaigneditor/startingbonus.cpp

@@ -55,9 +55,9 @@ void StartingBonus::initControls()
 	for(auto & hero : map->heroesOnMap)
 		if(hero->getOwner() == color || color == PlayerColor::CANNOT_DETERMINE)
 			heroSelection.emplace(hero->getHeroTypeID(), hero->getNameTranslated());
-	heroSelection.emplace(0xFFFD, tr("Strongest").toStdString());
+	heroSelection.emplace(HeroTypeID::STRONGEST, tr("Strongest").toStdString());
-	heroSelection.emplace(0xFFFE, tr("Generated").toStdString());
+	heroSelection.emplace(HeroTypeID::GENERATED, tr("Generated").toStdString());
-	heroSelection.emplace(0xFFFF, tr("Random").toStdString());
+	heroSelection.emplace(HeroTypeID::RANDOM, tr("Random").toStdString());
 
 	for(auto & hero : heroSelection)
 	{
@@ -106,15 +106,15 @@ void StartingBonus::initControls()
 	ui->comboBoxSecondarySkillMastery->addItem(tr("Advanced"), QVariant(1));
 	ui->comboBoxSecondarySkillMastery->addItem(tr("Expert"), QVariant(2));
 
-	ui->comboBoxResourceResourceType->addItem(tr("Wood"), QVariant(0));
+	ui->comboBoxResourceResourceType->addItem(tr("Wood"), QVariant(EGameResID::WOOD));
-	ui->comboBoxResourceResourceType->addItem(tr("Mercury"), QVariant(1));
+	ui->comboBoxResourceResourceType->addItem(tr("Mercury"), QVariant(EGameResID::MERCURY));
-	ui->comboBoxResourceResourceType->addItem(tr("Ore"), QVariant(2));
+	ui->comboBoxResourceResourceType->addItem(tr("Ore"), QVariant(EGameResID::ORE));
-	ui->comboBoxResourceResourceType->addItem(tr("Sulfur"), QVariant(3));
+	ui->comboBoxResourceResourceType->addItem(tr("Sulfur"), QVariant(EGameResID::SULFUR));
-	ui->comboBoxResourceResourceType->addItem(tr("Crystal"), QVariant(4));
+	ui->comboBoxResourceResourceType->addItem(tr("Crystal"), QVariant(EGameResID::CRYSTAL));
-	ui->comboBoxResourceResourceType->addItem(tr("Gems"), QVariant(5));
+	ui->comboBoxResourceResourceType->addItem(tr("Gems"), QVariant(EGameResID::GEMS));
-	ui->comboBoxResourceResourceType->addItem(tr("Gold"), QVariant(6));
+	ui->comboBoxResourceResourceType->addItem(tr("Gold"), QVariant(EGameResID::GOLD));
-	ui->comboBoxResourceResourceType->addItem(tr("Common (Wood and Ore)"), QVariant(0xFD));
+	ui->comboBoxResourceResourceType->addItem(tr("Common (Wood and Ore)"), QVariant(EGameResID::COMMON));
-	ui->comboBoxResourceResourceType->addItem(tr("Rare (Mercury, Sulfur, Crystal, Gems)"), QVariant(0xFE));
+	ui->comboBoxResourceResourceType->addItem(tr("Rare (Mercury, Sulfur, Crystal, Gems)"), QVariant(EGameResID::RARE));
 }
 
 void StartingBonus::loadBonus()
@@ -280,11 +280,11 @@ QString StartingBonus::getBonusListTitle(CampaignBonus bonus, std::shared_ptr<CM
 {
 	auto getHeroName = [](int id){
 		MetaString tmp;
-		if(id == 0xFFFD)
+		if(id == HeroTypeID::STRONGEST)
 			tmp.appendRawString(tr("strongest hero").toStdString());
-		else if(id == 0xFFFE)
+		else if(id == HeroTypeID::GENERATED)
 			tmp.appendRawString(tr("generated hero").toStdString());
-		else if(id == 0xFFFF)
+		else if(id == HeroTypeID::RANDOM)
 			tmp.appendRawString(tr("random hero").toStdString());
 		else
 			tmp.appendName(HeroTypeID(id));

mapeditor/mainwindow.cpp

@@ -321,7 +321,7 @@ void MainWindow::setStatusMessage(const QString & status)
 
 void MainWindow::setTitle()
 {
-	QString title = QString("%1%2 - %3 (%4)").arg(filename, unsaved ? "*" : "", VCMI_EDITOR_NAME, GameConstants::VCMI_VERSION.c_str());
+	QString title = QString("%1%2 - %3 (%4)").arg(filename, unsaved ? "*" : "", tr("VCMI Map Editor"), GameConstants::VCMI_VERSION.c_str());
 	setWindowTitle(title);
 }