Reworked and fixed selection of secondary skills:

- Fixed off-by-one error when checking for obligatory skills
- If both wisdom and magic school must be offered in the same slot, magic
school will be correctly offered on next levelup
- Obligatory skill can now be proposed for upgrade
- Obligatory skills are now offered using hero class weight instead of
simple random
- If hero has multiple skills not available to his class game will
select random skill instead of first one
- Moved storage of random seed to server instead of mutable member

lib/CHeroHandler.cpp

@@ -123,26 +123,30 @@ void CHero::serializeJson(JsonSerializeFormat & handler)
 
 SecondarySkill CHeroClass::chooseSecSkill(const std::set<SecondarySkill> & possibles, CRandomGenerator & rand) const //picks secondary skill out from given possibilities
 {
+	assert(!possibles.empty());
+
+	if (possibles.size() == 1)
+		return *possibles.begin();
+
 	int totalProb = 0;
 	for(const auto & possible : possibles)
 		if (secSkillProbability.count(possible) != 0)
 			totalProb += secSkillProbability.at(possible);
 
-	if (totalProb != 0) // may trigger if set contains only banned skills (0 probability)
+	if (totalProb == 0) // may trigger if set contains only banned skills (0 probability)
+		return *RandomGeneratorUtil::nextItem(possibles, rand);
+
+	auto ran = rand.nextInt(totalProb - 1);
+	for(const auto & possible : possibles)
 	{
-		auto ran = rand.nextInt(totalProb - 1);
-		for(const auto & possible : possibles)
-		{
-			if (secSkillProbability.count(possible) != 0)
-				ran -= secSkillProbability.at(possible);
+		if (secSkillProbability.count(possible) != 0)
+			ran -= secSkillProbability.at(possible);
 
-			if(ran < 0)
-			{
-				return possible;
-			}
-		}
+		if(ran < 0)
+			return possible;
 	}
-	// FIXME: select randomly? How H3 handles such rare situation?
+
+	assert(0); // should not be possible
 	return *possibles.begin();
 }
 

lib/mapObjects/CGHeroInstance.cpp

@@ -397,9 +397,7 @@ void CGHeroInstance::initHero(CRandomGenerator & rand)
 		commander->giveStackExp (exp); //after our exp is set
 	}
 
-	skillsInfo.rand.setSeed(rand.nextInt());
-	skillsInfo.resetMagicSchoolCounter();
-	skillsInfo.resetWisdomCounter();
+	skillsInfo = SecondarySkillsInfo();
 
 	//copy active (probably growing) bonuses from hero prototype to hero object
 	for(const std::shared_ptr<Bonus> & b : type->specialty)
@@ -569,17 +567,15 @@ ui8 CGHeroInstance::maxlevelsToWisdom() const
 CGHeroInstance::SecondarySkillsInfo::SecondarySkillsInfo():
 	magicSchoolCounter(1),
 	wisdomCounter(1)
-{
-	rand.setSeed(0);
-}
+{}
 
 void CGHeroInstance::SecondarySkillsInfo::resetMagicSchoolCounter()
 {
-	magicSchoolCounter = 1;
+	magicSchoolCounter = 0;
 }
 void CGHeroInstance::SecondarySkillsInfo::resetWisdomCounter()
 {
-	wisdomCounter = 1;
+	wisdomCounter = 0;
 }
 
 void CGHeroInstance::pickRandomObject(CRandomGenerator & rand)
@@ -1268,49 +1264,31 @@ ArtBearer::ArtBearer CGHeroInstance::bearerType() const
 	return ArtBearer::HERO;
 }
 
-std::vector<SecondarySkill> CGHeroInstance::getLevelUpProposedSecondarySkills() const
+std::vector<SecondarySkill> CGHeroInstance::getLevelUpProposedSecondarySkills(CRandomGenerator & rand) const
 {
-	std::vector<SecondarySkill> obligatorySkills; //hero is offered magic school or wisdom if possible
-
 	auto getObligatorySkills = [](CSkill::Obligatory obl){
-		std::vector<SecondarySkill> obligatory = {};
+		std::set<SecondarySkill> obligatory;
 		for(auto i = 0; i < VLC->skillh->size(); i++)
 			if((*VLC->skillh)[SecondarySkill(i)]->obligatory(obl))
-				obligatory.emplace_back(i); //Always return all obligatory skills
+				obligatory.insert(i); //Always return all obligatory skills
 
 		return obligatory;
 	};
 
-	auto selectObligatorySkill = [&](std::vector<SecondarySkill>& ss) -> void
+	auto intersect = [](const std::set<SecondarySkill> & left, const std::set<SecondarySkill> & right)
 	{
-		std::shuffle(ss.begin(), ss.end(), skillsInfo.rand.getStdGenerator());
-
-		for(const auto & skill : ss)
-		{
-			if (canLearnSkill(skill)) //only skills hero doesn't know yet
-			{
-				obligatorySkills.push_back(skill);
-				break; //only one
-			}
-		}
+		std::set<SecondarySkill> intersect;
+		std::set_intersection(left.begin(), left.end(), right.begin(), right.end(),
+						 std::inserter(intersect, intersect.begin()));
+		return intersect;
 	};
 
-	if (!skillsInfo.wisdomCounter)
-	{
-		auto obligatory = getObligatorySkills(CSkill::Obligatory::MAJOR);
-		selectObligatorySkill(obligatory);
-	}
-	if (!skillsInfo.magicSchoolCounter)
-	{
-		auto obligatory = getObligatorySkills(CSkill::Obligatory::MINOR);
-		selectObligatorySkill(obligatory);
-	}
+	std::set<SecondarySkill> wisdomList = getObligatorySkills(CSkill::Obligatory::MAJOR);
+	std::set<SecondarySkill> schoolList = getObligatorySkills(CSkill::Obligatory::MINOR);
 
-	std::vector<SecondarySkill> skills;
-	//picking sec. skills for choice
 	std::set<SecondarySkill> basicAndAdv;
-	std::set<SecondarySkill> expert;
 	std::set<SecondarySkill> none;
+
 	for(int i = 0; i < VLC->skillh->size(); i++)
 		if (canLearnSkill(SecondarySkill(i)))
 			none.insert(SecondarySkill(i));
@@ -1319,58 +1297,56 @@ std::vector<SecondarySkill> CGHeroInstance::getLevelUpProposedSecondarySkills()
 	{
 		if(elem.second < MasteryLevel::EXPERT)
 			basicAndAdv.insert(elem.first);
-		else
-			expert.insert(elem.first);
 		none.erase(elem.first);
 	}
-	for(const auto & s : obligatorySkills) //don't duplicate them
-	{
-		none.erase (s);
-		basicAndAdv.erase (s);
-		expert.erase (s);
-	}
 
-	//first offered skill:
-	// 1) give obligatory skill
-	// 2) give any other new skill
-	// 3) upgrade existing
-	if(canLearnSkill() && !obligatorySkills.empty())
-	{
-		skills.push_back (obligatorySkills[0]);
-	}
-	else if(!none.empty() && canLearnSkill()) //hero have free skill slot
-	{
-		skills.push_back(type->heroClass->chooseSecSkill(none, skillsInfo.rand)); //new skill
-		none.erase(skills.back());
-	}
-	else if(!basicAndAdv.empty())
-	{
-		skills.push_back(type->heroClass->chooseSecSkill(basicAndAdv, skillsInfo.rand)); //upgrade existing
-		basicAndAdv.erase(skills.back());
-	}
+	bool wantsWisdom = skillsInfo.wisdomCounter + 1 >= maxlevelsToWisdom();
+	bool wantsSchool = skillsInfo.magicSchoolCounter + 1 >= maxlevelsToMagicSchool();
 
-	//second offered skill:
-	//1) upgrade existing
-	//2) give obligatory skill
-	//3) give any other new skill
-	if(!basicAndAdv.empty())
-	{
-		SecondarySkill s = type->heroClass->chooseSecSkill(basicAndAdv, skillsInfo.rand);//upgrade existing
-		skills.push_back(s);
-		basicAndAdv.erase(s);
-	}
-	else if (canLearnSkill() && obligatorySkills.size() > 1)
-	{
-		skills.push_back (obligatorySkills[1]);
-	}
-	else if(!none.empty() && canLearnSkill())
+	std::vector<SecondarySkill> skills;
+
+	auto chooseSkill = [&](std::set<SecondarySkill> & options)
 	{
-		skills.push_back(type->heroClass->chooseSecSkill(none, skillsInfo.rand)); //give new skill
-		none.erase(skills.back());
-	}
+		bool selectWisdom = wantsWisdom && !intersect(options, wisdomList).empty();
+		bool selectSchool = wantsSchool && !intersect(options, schoolList).empty();
+		SecondarySkill selection;
+
+		if (selectWisdom)
+			selection = type->heroClass->chooseSecSkill(intersect(options, wisdomList), rand);
+		else if (selectSchool)
+			selection = type->heroClass->chooseSecSkill(intersect(options, schoolList), rand);
+		else
+			selection = type->heroClass->chooseSecSkill(options, rand);
+
+		skills.push_back(selection);
+		options.erase(selection);
+
+		if (wisdomList.count(selection))
+			wisdomList.clear();
+
+		if (schoolList.count(selection))
+			schoolList.clear();
+	};
+
+	if (!basicAndAdv.empty())
+		chooseSkill(basicAndAdv);
+
+	if (canLearnSkill() && !none.empty())
+		chooseSkill(none);
+
+	if (!basicAndAdv.empty() && skills.size() < 2)
+		chooseSkill(basicAndAdv);
+
+	if (canLearnSkill() && !none.empty() && skills.size() < 2)
+		chooseSkill(none);
+
+	if (skills.empty())
+		logGlobal->info("Selecting secondary skills: Nothing to select!");
+	if (skills.size() == 1)
+		logGlobal->info("Selecting secondary skills: %s (wisdom: %d, schools: %d)!", skills[0], skillsInfo.wisdomCounter, skillsInfo.magicSchoolCounter);
+	if (skills.size() == 2)
+		logGlobal->info("Selecting secondary skills: %s or %s (wisdom: %d, schools: %d)!", skills[0], skills[1], int(skillsInfo.wisdomCounter), int(skillsInfo.magicSchoolCounter));
 
-	if (skills.size() == 2) // Fix for #1868 to avoid changing logic (possibly causing bugs in process)
-		std::swap(skills[0], skills[1]);
 	return skills;
 }
 
@@ -1406,7 +1382,7 @@ std::optional<SecondarySkill> CGHeroInstance::nextSecondarySkill(CRandomGenerato
 	assert(gainsLevel());
 
 	std::optional<SecondarySkill> chosenSecondarySkill;
-	const auto proposedSecondarySkills = getLevelUpProposedSecondarySkills();
+	const auto proposedSecondarySkills = getLevelUpProposedSecondarySkills(rand);
 	if(!proposedSecondarySkills.empty())
 	{
 		std::vector<SecondarySkill> learnedSecondarySkills;
@@ -1474,8 +1450,9 @@ void CGHeroInstance::levelUp(const std::vector<SecondarySkill> & skills)
 	++level;
 
 	//deterministic secondary skills
-	skillsInfo.magicSchoolCounter = (skillsInfo.magicSchoolCounter + 1) % maxlevelsToMagicSchool();
-	skillsInfo.wisdomCounter = (skillsInfo.wisdomCounter + 1) % maxlevelsToWisdom();
+	++skillsInfo.magicSchoolCounter;
+	++skillsInfo.wisdomCounter;
+
 	for(const auto & skill : skills)
 	{
 		if((*VLC->skillh)[skill]->obligatory(CSkill::Obligatory::MAJOR))
@@ -1495,7 +1472,7 @@ void CGHeroInstance::levelUpAutomatically(CRandomGenerator & rand)
 		const auto primarySkill = nextPrimarySkill(rand);
 		setPrimarySkill(primarySkill, 1, false);
 
-		auto proposedSecondarySkills = getLevelUpProposedSecondarySkills();
+		auto proposedSecondarySkills = getLevelUpProposedSecondarySkills(rand);
 
 		const auto secondarySkill = nextSecondarySkill(rand);
 		if(secondarySkill)

lib/mapObjects/CGHeroInstance.h

@@ -111,9 +111,6 @@ public:
 
 	struct DLL_LINKAGE SecondarySkillsInfo
 	{
-		//skills are determined, initialized at map start
-		//FIXME remove mutable
-		mutable CRandomGenerator rand;
 		ui8 magicSchoolCounter;
 		ui8 wisdomCounter;
 
@@ -126,7 +123,6 @@ public:
 		{
 			h & magicSchoolCounter;
 			h & wisdomCounter;
-			h & rand;
 		}
 	} skillsInfo;
 
@@ -194,7 +190,7 @@ public:
 	std::optional<SecondarySkill> nextSecondarySkill(CRandomGenerator & rand) const;
 
 	/// Gets 0, 1 or 2 secondary skills which are proposed on hero level up.
-	std::vector<SecondarySkill> getLevelUpProposedSecondarySkills() const;
+	std::vector<SecondarySkill> getLevelUpProposedSecondarySkills(CRandomGenerator & rand) const;
 
 	ui8 getSecSkillLevel(const SecondarySkill & skill) const; //0 - no skill
 

lib/networkPacks/NetPackVisitor.h

@@ -87,7 +87,6 @@ public:
 	virtual void visitInfoWindow(InfoWindow & pack) {}
 	virtual void visitSetObjectProperty(SetObjectProperty & pack) {}
 	virtual void visitChangeObjectVisitors(ChangeObjectVisitors & pack) {}
-	virtual void visitPrepareHeroLevelUp(PrepareHeroLevelUp & pack) {}
 	virtual void visitHeroLevelUp(HeroLevelUp & pack) {}
 	virtual void visitCommanderLevelUp(CommanderLevelUp & pack) {}
 	virtual void visitBlockingDialog(BlockingDialog & pack) {}

lib/networkPacks/NetPacksLib.cpp

@@ -380,11 +380,6 @@ void ChangeObjectVisitors::visitTyped(ICPackVisitor & visitor)
 	visitor.visitChangeObjectVisitors(*this);
 }
 
-void PrepareHeroLevelUp::visitTyped(ICPackVisitor & visitor)
-{
-	visitor.visitPrepareHeroLevelUp(*this);
-}
-
 void HeroLevelUp::visitTyped(ICPackVisitor & visitor)
 {
 	visitor.visitHeroLevelUp(*this);
@@ -2082,23 +2077,6 @@ void SetObjectProperty::applyGs(CGameState * gs) const
 	}
 }
 
-void PrepareHeroLevelUp::applyGs(CGameState * gs)
-{
-	auto * hero = gs->getHero(heroId);
-	assert(hero);
-
-	auto proposedSkills = hero->getLevelUpProposedSecondarySkills();
-
-	if(skills.size() == 1 || hero->tempOwner == PlayerColor::NEUTRAL) //choose skill automatically
-	{
-		skills.push_back(*RandomGeneratorUtil::nextItem(proposedSkills, hero->skillsInfo.rand));
-	}
-	else
-	{
-		skills = proposedSkills;
-	}
-}
-
 void HeroLevelUp::applyGs(CGameState * gs) const
 {
 	auto * hero = gs->getHero(heroId);

lib/networkPacks/PacksForClient.h

@@ -1276,23 +1276,6 @@ struct DLL_LINKAGE ChangeObjectVisitors : public CPackForClient
 	}
 };
 
-struct DLL_LINKAGE PrepareHeroLevelUp : public CPackForClient
-{
-	ObjectInstanceID heroId;
-
-	/// Do not serialize, used by server only
-	std::vector<SecondarySkill> skills;
-
-	void applyGs(CGameState * gs);
-
-	void visitTyped(ICPackVisitor & visitor) override;
-
-	template <typename Handler> void serialize(Handler & h, const int version)
-	{
-		h & heroId;
-	}
-};
-
 struct DLL_LINKAGE HeroLevelUp : public Query
 {
 	PlayerColor player;

lib/registerTypes/RegisterTypesClientPacks.h

@@ -68,7 +68,6 @@ void registerTypesClientPacks(Serializer &s)
 	s.template registerType<CPackForClient, SetCommanderProperty>();
 	s.template registerType<CPackForClient, ChangeObjectVisitors>();
 	s.template registerType<CPackForClient, ShowWorldViewEx>();
-	s.template registerType<CPackForClient, PrepareHeroLevelUp>();
 	s.template registerType<CPackForClient, EntitiesChanged>();
 	s.template registerType<CPackForClient, BattleStart>();
 	s.template registerType<CPackForClient, BattleNextRound>();

server/CGameHandler.cpp

@@ -187,25 +187,21 @@ void CGameHandler::levelUpHero(const CGHeroInstance * hero)
 	sps.val = 1;
 	sendAndApply(&sps);
 
-	PrepareHeroLevelUp pre;
-	pre.heroId = hero->id;
-	sendAndApply(&pre);
-
 	HeroLevelUp hlu;
 	hlu.player = hero->tempOwner;
 	hlu.heroId = hero->id;
 	hlu.primskill = primarySkill;
-	hlu.skills = pre.skills;
+	hlu.skills = hero->getLevelUpProposedSecondarySkills(heroPool->getHeroSkillsRandomGenerator(hero->getHeroType()));
 
 	if (hlu.skills.size() == 0)
 	{
 		sendAndApply(&hlu);
 		levelUpHero(hero);
 	}
-	else if (hlu.skills.size() == 1)
+	else if (hlu.skills.size() == 1 || !hero->getOwner().isValidPlayer())
 	{
 		sendAndApply(&hlu);
-		levelUpHero(hero, pre.skills.front());
+		levelUpHero(hero, hlu.skills.front());
 	}
 	else if (hlu.skills.size() > 1)
 	{
@@ -551,6 +547,9 @@ void CGameHandler::init(StartInfo *si, Load::ProgressAccumulator & progressTrack
 	for (auto & elem : gs->players)
 		turnOrder->addPlayer(elem.first);
 
+	for (auto & elem : gs->map->allHeroes)
+		heroPool->getHeroSkillsRandomGenerator(elem->getHeroType()); // init RMG seed
+
 	reinitScripting();
 }
 

server/processors/HeroPoolProcessor.cpp

@@ -351,6 +351,17 @@ CGHeroInstance * HeroPoolProcessor::pickHeroFor(bool isNative, const PlayerColor
 	return *RandomGeneratorUtil::nextItem(possibleHeroes, getRandomGenerator(player));
 }
 
+CRandomGenerator & HeroPoolProcessor::getHeroSkillsRandomGenerator(const HeroTypeID & hero)
+{
+	if (heroSeed.count(hero) == 0)
+	{
+		int seed = gameHandler->getRandomGenerator().nextInt();
+		heroSeed.emplace(hero, std::make_unique<CRandomGenerator>(seed));
+	}
+
+	return *heroSeed.at(hero);
+}
+
 CRandomGenerator & HeroPoolProcessor::getRandomGenerator(const PlayerColor & player)
 {
 	if (playerSeed.count(player) == 0)

server/processors/HeroPoolProcessor.h

@@ -29,6 +29,9 @@ class HeroPoolProcessor : boost::noncopyable
 	/// per-player random generators
 	std::map<PlayerColor, std::unique_ptr<CRandomGenerator>> playerSeed;
 
+	/// per-hero random generators used to randomize skills
+	std::map<HeroTypeID, std::unique_ptr<CRandomGenerator>> heroSeed;
+
 	void clearHeroFromSlot(const PlayerColor & color, TavernHeroSlot slot);
 	void selectNewHeroForSlot(const PlayerColor & color, TavernHeroSlot slot, bool needNativeHero, bool giveStartingArmy);
 
@@ -54,6 +57,8 @@ public:
 
 	void onNewWeek(const PlayerColor & color);
 
+	CRandomGenerator & getHeroSkillsRandomGenerator(const HeroTypeID & hero);
+
 	/// Incoming net pack handling
 	bool hireHero(const ObjectInstanceID & objectID, const HeroTypeID & hid, const PlayerColor & player);
 
@@ -61,5 +66,6 @@ public:
 	{
 		// h & gameHandler; // FIXME: make this work instead of using deserializationFix in gameHandler
 		h & playerSeed;
+		h & heroSeed;
 	}
 };