code review

client/CServerHandler.cpp

@@ -497,7 +497,7 @@ void CServerHandler::setPlayerName(PlayerColor color, const std::string & name)
 	sendLobbyPack(lspn);
 }
 
-void CServerHandler::setPlayerHandicap(PlayerColor color, PlayerSettings::Handicap handicap) const
+void CServerHandler::setPlayerHandicap(PlayerColor color, Handicap handicap) const
 {
 	LobbySetPlayerHandicap lsph;
 	lsph.color = color;

client/CServerHandler.h

@@ -81,7 +81,7 @@ public:
 	virtual void setMapInfo(std::shared_ptr<CMapInfo> to, std::shared_ptr<CMapGenOptions> mapGenOpts = {}) const = 0;
 	virtual void setPlayer(PlayerColor color) const = 0;
 	virtual void setPlayerName(PlayerColor color, const std::string & name) const = 0;
-	virtual void setPlayerHandicap(PlayerColor color, PlayerSettings::Handicap handicap) const = 0;
+	virtual void setPlayerHandicap(PlayerColor color, Handicap handicap) const = 0;
 	virtual void setPlayerOption(ui8 what, int32_t value, PlayerColor player) const = 0;
 	virtual void setDifficulty(int to) const = 0;
 	virtual void setTurnTimerInfo(const TurnTimerInfo &) const = 0;
@@ -192,7 +192,7 @@ public:
 	void setMapInfo(std::shared_ptr<CMapInfo> to, std::shared_ptr<CMapGenOptions> mapGenOpts = {}) const override;
 	void setPlayer(PlayerColor color) const override;
 	void setPlayerName(PlayerColor color, const std::string & name) const override;
-	void setPlayerHandicap(PlayerColor color, PlayerSettings::Handicap handicap) const override;
+	void setPlayerHandicap(PlayerColor color, Handicap handicap) const override;
 	void setPlayerOption(ui8 what, int32_t value, PlayerColor player) const override;
 	void setDifficulty(int to) const override;
 	void setTurnTimerInfo(const TurnTimerInfo &) const override;

client/lobby/OptionsTab.cpp

@@ -843,6 +843,7 @@ OptionsTab::HandicapWindow::HandicapWindow()
 			textinputs[player][resource] = std::make_shared<CTextInput>(area, FONT_SMALL, ETextAlignment::CENTERLEFT, true);
 			textinputs[player][resource]->setText(std::to_string(isIncome ? ps.handicap.percentIncome : (isGrowth ? ps.handicap.percentGrowth : ps.handicap.startBonus[resource])));
 			textinputs[player][resource]->setCallback([this, player, resource, isIncome, isGrowth](const std::string & s){
+				// text input processing: add/remove sign when pressing "-"; remove non digits; cut length; fill empty field with 0
 				std::string tmp = s;
 				bool negative = std::count_if( s.begin(), s.end(), []( char c ){ return c == '-'; }) == 1 && !isIncome && !isGrowth;
 				tmp.erase(std::remove_if(tmp.begin(), tmp.end(), [](char c) { return !isdigit(c); }), tmp.end());
@@ -850,6 +851,7 @@ OptionsTab::HandicapWindow::HandicapWindow()
 				textinputs[player][resource]->setText(tmp.length() == 0 ? "0" : (negative ? "-" : "") + std::to_string(stoi(tmp)));
 			});
 			textinputs[player][resource]->setPopupCallback([isIncome, isGrowth](){
+				// Help for the textinputs
 				if(isIncome)
 					CRClickPopup::createAndPush(CGI->generaltexth->translate("vcmi.lobby.handicap.income"));
 				else if(isGrowth)
@@ -880,7 +882,7 @@ OptionsTab::HandicapWindow::HandicapWindow()
 				else
 					resources[resource.first] = std::stoi(resource.second->getText());
 			}
-			CSH->setPlayerHandicap(player.first, PlayerSettings::Handicap{resources, income, growth});
+			CSH->setPlayerHandicap(player.first, Handicap{resources, income, growth});
 		}
 	    		
 		close();
@@ -897,7 +899,7 @@ bool OptionsTab::HandicapWindow::receiveEvent(const Point & position, int eventT
 
 void OptionsTab::HandicapWindow::clickReleased(const Point & cursorPosition)
 {
-	if(!pos.isInside(cursorPosition))
+	if(!pos.isInside(cursorPosition)) // make it possible to close window by touching/clicking outside of window
 		close();
 }
 
@@ -1031,14 +1033,16 @@ OptionsTab::PlayerOptionsEntry::PlayerOptionsEntry(const PlayerSettings & S, con
 	}
 	labelWhoCanPlay = std::make_shared<CMultiLineLabel>(Rect(6, 23, 45, (int)graphics->fonts[EFonts::FONT_TINY]->getLineHeight()*2), EFonts::FONT_TINY, ETextAlignment::CENTER, Colors::WHITE, CGI->generaltexth->arraytxt[206 + whoCanPlay]);
 
-	labelHandicap = std::make_shared<CMultiLineLabel>(Rect(57, 24, 47, (int)graphics->fonts[EFonts::FONT_TINY]->getLineHeight()*2), EFonts::FONT_TINY, ETextAlignment::CENTER, Colors::WHITE, s->handicap.startBonus.empty() && s->handicap.percentIncome == 100 && s->handicap.percentGrowth == 100 ? CGI->generaltexth->arraytxt[210] : MetaString::createFromTextID("vcmi.lobby.handicap").toString());
+	auto hasHandicap = [this](){ return s->handicap.startBonus.empty() && s->handicap.percentIncome == 100 && s->handicap.percentGrowth == 100; };
+	std::string labelHandicapText = hasHandicap() ? CGI->generaltexth->arraytxt[210] : MetaString::createFromTextID("vcmi.lobby.handicap").toString();
+	labelHandicap = std::make_shared<CMultiLineLabel>(Rect(57, 24, 47, (int)graphics->fonts[EFonts::FONT_TINY]->getLineHeight()*2), EFonts::FONT_TINY, ETextAlignment::CENTER, Colors::WHITE, labelHandicapText);
 	handicap = std::make_shared<LRClickableArea>(Rect(56, 24, 49, (int)graphics->fonts[EFonts::FONT_TINY]->getLineHeight()*2), [](){
 		if(!CSH->isHost())
 			return;
 		
 		GH.windows().createAndPushWindow<HandicapWindow>();
-	}, [this](){
+	}, [this, hasHandicap](){
-		if(s->handicap.startBonus.empty() && s->handicap.percentIncome == 100 && s->handicap.percentGrowth == 100)
+		if(hasHandicap())
 			CRClickPopup::createAndPush(MetaString::createFromTextID("core.help.124.help").toString());
 		else
 		{

config/shortcutsConfig.json

@@ -150,11 +150,11 @@
 		"lobbyRandomMap":           "R",
 		"lobbyRandomTown":          "T",
 		"lobbyRandomTownVs":        "V",
-		"lobbyHandicap":            "C",
+		"lobbyHandicap":            "H",
 		"lobbyReplayVideo":         "R",
 		"lobbySaveGame":            [ "S", "Return", "Keypad Enter"],
 		"lobbySelectScenario":      "S",
-		"lobbyToggleChat":          "H",
+		"lobbyToggleChat":          "C",
 		"lobbyTurnOptions":         "T",
 		"mainMenuBack":             [ "B", "Escape" ],
 		"mainMenuCampaign":         "C",

lib/StartInfo.cpp

@@ -25,7 +25,7 @@
 VCMI_LIB_NAMESPACE_BEGIN
 
 PlayerSettings::PlayerSettings()
-	: bonus(PlayerStartingBonus::RANDOM), color(0), handicap({TResources(), 100, 100}), compOnly(false)
+	: bonus(PlayerStartingBonus::RANDOM), color(0), compOnly(false)
 {
 }
 

lib/StartInfo.h

@@ -66,6 +66,20 @@ enum class PlayerStartingBonus : int8_t
 	RESOURCE =  2
 };
 
+struct DLL_LINKAGE Handicap {
+	TResources startBonus = TResources();
+	int percentIncome = 100;
+	int percentGrowth = 100;
+
+	template <typename Handler>
+	void serialize(Handler &h)
+	{
+		h & startBonus;
+		h & percentIncome;
+		h & percentGrowth;
+	}
+};
+
 /// Struct which describes the name, the color, the starting bonus of a player
 struct DLL_LINKAGE PlayerSettings
 {
@@ -78,13 +92,8 @@ struct DLL_LINKAGE PlayerSettings
 
 	std::string heroNameTextId;
 	PlayerColor color; //from 0 -
-	enum EHandicap {NO_HANDICAP, MILD, SEVERE};
+
-	EHandicap handicapLegacy;//0-no, 1-mild, 2-severe
+	Handicap handicap;
-	struct Handicap {
-		TResources startBonus;
-		int percentIncome;
-		int percentGrowth;
-	} handicap;
 
 	std::string name;
 	std::set<ui8> connectedPlayerIDs; //Empty - AI, or connectrd player ids
@@ -99,13 +108,13 @@ struct DLL_LINKAGE PlayerSettings
 		h & bonus;
 		h & color;
 		if (h.version >= Handler::Version::PLAYER_HANDICAP)
-		{
+			h & handicap;
-			h & handicap.startBonus;
-			h & handicap.percentIncome;
-			h & handicap.percentGrowth;
-		}
 		else
+		{
+			enum EHandicap {NO_HANDICAP, MILD, SEVERE};
+			EHandicap handicapLegacy;
 			h & handicapLegacy;
+		}
 		h & name;
 		h & connectedPlayerIDs;
 		h & compOnly;

lib/mapObjects/CGTownInstance.cpp

@@ -140,10 +140,10 @@ GrowthInfo CGTownInstance::getGrowthInfo(int level) const
 	if(tempOwner.isValidPlayer())
 	{
 		auto * playerSettings = cb->getPlayerSettings(tempOwner);
-		ret.percent = playerSettings->handicap.percentGrowth;
+		ret.handicapPercentage = playerSettings->handicap.percentGrowth;
 	}
 	else
-		ret.percent = 100;
+		ret.handicapPercentage = 100;
 
 	ret.entries.emplace_back(VLC->generaltexth->allTexts[590], base); // \n\nBasic growth %d"
 
@@ -226,7 +226,8 @@ TResources CGTownInstance::dailyIncome() const
 
 	auto playerSettings = cb->gameState()->scenarioOps->getIthPlayersSettings(getOwner());
 	for(TResources::nziterator it(ret); it.valid(); it++)
-		ret[it->resType] = ret[it->resType] * playerSettings.handicap.percentIncome / 100;
+		// always round up income - we don't want to always produce zero if handicap in use
+		ret[it->resType] = (ret[it->resType] * playerSettings.handicap.percentIncome + 99) / 100;
 	return ret;
 }
 
@@ -1269,10 +1270,8 @@ int GrowthInfo::totalGrowth() const
 	for(const Entry &entry : entries)
 		ret += entry.count;
 
-	auto retCalc = ret * percent / 100;
+	// always round up income - we don't want buildings to always produce zero if handicap in use
-	if(retCalc == 0 && ret > 0) //generate at least one
+	return (ret * handicapPercentage + 99) / 100;
-		retCalc = 1;
-	return retCalc;
 }
 
 void CGTownInstance::fillUpgradeInfo(UpgradeInfo & info, const CStackInstance &stack) const

lib/mapObjects/CGTownInstance.h

@@ -41,7 +41,7 @@ struct DLL_LINKAGE GrowthInfo
 
 	std::vector<Entry> entries;
 	int totalGrowth() const;
-	int percent;
+	int handicapPercentage;
 };
 
 class DLL_LINKAGE CGTownInstance : public CGDwelling, public IShipyard, public IMarket, public INativeTerrainProvider, public ICreatureUpgrader

lib/mapObjects/MiscObjects.cpp

@@ -199,10 +199,8 @@ ui32 CGMine::defaultResProduction() const
 ui32 CGMine::getProducedQuantity() const
 {
 	auto * playerSettings = cb->getPlayerSettings(getOwner());
-	auto ret = producedQuantity * playerSettings->handicap.percentIncome / 100;
+	// always round up income - we don't want mines to always produce zero if handicap in use
-	if(ret == 0 && producedQuantity > 0) // create at least 1 resource
+	return (producedQuantity * playerSettings->handicap.percentIncome + 99) / 100;
-		ret = 1;
-	return ret;
 }
 
 void CGMine::battleFinished(const CGHeroInstance *hero, const BattleResult &result) const

lib/networkPacks/PacksForLobby.h

@@ -284,16 +284,14 @@ struct DLL_LINKAGE LobbySetPlayerName : public CLobbyPackToServer
 struct DLL_LINKAGE LobbySetPlayerHandicap : public CLobbyPackToServer
 {
 	PlayerColor color = PlayerColor::CANNOT_DETERMINE;
-	PlayerSettings::Handicap handicap = PlayerSettings::Handicap();
+	Handicap handicap = Handicap();
 
 	void visitTyped(ICPackVisitor & visitor) override;
 
 	template <typename Handler> void serialize(Handler &h)
 	{
 		h & color;
-		h & handicap.startBonus;
+		h & handicap;
-		h & handicap.percentIncome;
-		h & handicap.percentGrowth;
 	}
 };
 

server/CVCMIServer.cpp

@@ -624,8 +624,6 @@ void CVCMIServer::updateStartInfoOnMapChange(std::shared_ptr<CMapInfo> mapInfo,
 				pset.heroNameTextId = pinfo.mainCustomHeroNameTextId;
 				pset.heroPortrait = pinfo.mainCustomHeroPortrait;
 			}
-
-			pset.handicapLegacy = PlayerSettings::NO_HANDICAP;
 		}
 
 		if(mi->isRandomMap && mapGenOpts)
@@ -765,7 +763,7 @@ void CVCMIServer::setPlayerName(PlayerColor color, std::string name)
 	setPlayerConnectedId(player, nameID);
 }
 
-void CVCMIServer::setPlayerHandicap(PlayerColor color, PlayerSettings::Handicap handicap)
+void CVCMIServer::setPlayerHandicap(PlayerColor color, Handicap handicap)
 {
 	if(color == PlayerColor::CANNOT_DETERMINE)
 		return;

server/CVCMIServer.h

@@ -117,7 +117,7 @@ public:
 	// Work with LobbyInfo
 	void setPlayer(PlayerColor clickedColor);
 	void setPlayerName(PlayerColor player, std::string name);
-	void setPlayerHandicap(PlayerColor player, PlayerSettings::Handicap handicap);
+	void setPlayerHandicap(PlayerColor player, Handicap handicap);
 	void optionNextHero(PlayerColor player, int dir); //dir == -1 or +1
 	void optionSetHero(PlayerColor player, HeroTypeID id);
 	HeroTypeID nextAllowedHero(PlayerColor player, HeroTypeID id, int direction);

test/game/CGameStateTest.cpp

@@ -175,8 +175,6 @@ public:
 				pset.heroNameTextId = pinfo.mainCustomHeroNameTextId;
 				pset.heroPortrait = HeroTypeID(pinfo.mainCustomHeroPortrait);
 			}
-
-			pset.handicapLegacy = PlayerSettings::NO_HANDICAP;
 		}