Refactor CGHeroInstance, make spells private

AI/VCAI/Goals/AdventureSpellCast.cpp

@@ -38,10 +38,10 @@ TSubgoal AdventureSpellCast::whatToDoToAchieve()
 	if(!spell->isAdventureSpell())
 		throw cannotFulfillGoalException(spell->name + " is not an adventure spell.");
 
-	if(!vstd::contains(hero->spells, spellID))
-		throw cannotFulfillGoalException("Hero has no " + spell->name);
+	if(!hero->canCastThisSpell(spell))
+		throw cannotFulfillGoalException("Hero can not cast " + spell->name);
 
-	if(hero->mana < hero->getSpellCost(spellID.toSpell()))
+	if(hero->mana < hero->getSpellCost(spell))
 		throw cannotFulfillGoalException("Hero has not enough mana to cast " + spell->name);
 
 	return iAmElementar();

AI/VCAI/Goals/Explore.cpp

@@ -46,7 +46,7 @@ TSubgoal Explore::whatToDoToAchieve()
 	else
 	{
 		if(ret->hero.get(true))
-			return sptr(sethero(ret->hero.h).setisAbstract(true)); //choose this hero and then continue with him
+			return sptr(Explore().sethero(ret->hero.h)); //choose this hero and then continue with him
 		else
 			return ret; //other solutions, like buying hero from tavern
 	}

AI/VCAI/Pathfinding/AIPathfinderConfig.cpp

@@ -184,15 +184,13 @@ namespace AIPathfinding
 			}
 
 			auto hero = nodeStorage->getHero();
+			auto summonBoatSpell = SpellID(SpellID::SUMMON_BOAT).toSpell();
 
-			if(vstd::contains(hero->spells, SpellID::SUMMON_BOAT))
+			if(hero->canCastThisSpell(summonBoatSpell)
+				&& hero->getSpellSchoolLevel(summonBoatSpell) >= SecSkillLevel::ADVANCED)
 			{
-				auto summonBoatSpell = SpellID(SpellID::SUMMON_BOAT).toSpell();
-
-				if(hero->getSpellSchoolLevel(summonBoatSpell) == SecSkillLevel::EXPERT)
-				{
-					summonableVirtualBoat.reset(new SummonBoatAction());
-				}
+				// TODO: For lower school level we might need to check the existance of some boat
+				summonableVirtualBoat.reset(new SummonBoatAction());
 			}
 		}
 

AI/VCAI/VCAI.cpp

@@ -2404,7 +2404,6 @@ Goals::TSubgoal VCAI::decomposeGoal(Goals::TSubgoal ultimateGoal)
 	}
 
 	const int searchDepth = 30;
-	Goals::TSubgoal abstractGoal = sptr(Goals::Invalid());
 
 	Goals::TSubgoal goal = ultimateGoal;
 	logAi->debug("Decomposing goal %s", ultimateGoal->name());
@@ -2424,16 +2423,8 @@ Goals::TSubgoal VCAI::decomposeGoal(Goals::TSubgoal ultimateGoal)
 		else
 			logAi->debug("Considering: %s", goal->name());
 	}
-	if (maxGoals <= 0)
-	{
-		throw cannotFulfillGoalException("Too many subgoals, don't know what to do");
-	}
-	else
-	{
-		return goal;
-	}
 
-	return abstractGoal;
+	throw cannotFulfillGoalException("Too many subgoals, don't know what to do");
 }
 
 void VCAI::performTypicalActions()

lib/CGameState.cpp

@@ -1195,12 +1195,7 @@ void CGameState::prepareCrossoverHeroes(std::vector<CGameState::CampaignHeroRepl
 	{
 		for(CGHeroInstance * cgh : crossoverHeroes)
 		{
-			// Trimming spells
-			cgh->spells.clear();
-
-			// Spellbook will also be removed
-			if (cgh->hasSpellbook())
-				ArtifactLocation(cgh, ArtifactPosition(ArtifactPosition::SPELLBOOK)).removeArtifact();
+			cgh->removeSpellbook();
 		}
 	}
 
@@ -1469,7 +1464,7 @@ void CGameState::giveCampaignBonusToHero(CGHeroInstance * hero)
 		switch (curBonus->type)
 		{
 		case CScenarioTravel::STravelBonus::SPELL:
-			hero->spells.insert(SpellID(curBonus->info2));
+			hero->addSpellToSpellbook(SpellID(curBonus->info2));
 			break;
 		case CScenarioTravel::STravelBonus::MONSTER:
 			{

lib/NetPacksLib.cpp

@@ -140,10 +140,10 @@ DLL_LINKAGE void ChangeSpells::applyGs(CGameState *gs)
 
 	if(learn)
 		for(auto sid : spells)
-			hero->spells.insert(sid);
+			hero->addSpellToSpellbook(sid);
 	else
 		for(auto sid : spells)
-			hero->spells.erase(sid);
+			hero->removeSpellFromSpellbook(sid);
 }
 
 DLL_LINKAGE void SetMana::applyGs(CGameState *gs)

lib/mapObjects/CGHeroInstance.cpp

@@ -1020,6 +1020,36 @@ bool CGHeroInstance::hasSpellbook() const
 	return getArt(ArtifactPosition::SPELLBOOK);
 }
 
+void CGHeroInstance::addSpellToSpellbook(SpellID spell)
+{
+	spells.insert(spell);
+}
+
+void CGHeroInstance::removeSpellFromSpellbook(SpellID spell)
+{
+	spells.erase(spell);
+}
+
+bool CGHeroInstance::spellbookContainsSpell(SpellID spell) const
+{
+	return vstd::contains(spells, spell);
+}
+
+void CGHeroInstance::removeSpellbook()
+{
+	spells.clear();
+
+	if(hasSpellbook())
+	{
+		ArtifactLocation(this, ArtifactPosition(ArtifactPosition::SPELLBOOK)).removeArtifact();
+	}
+}
+
+const std::set<SpellID> & CGHeroInstance::getSpellsInSpellbook() const
+{
+	return spells;
+}
+
 int CGHeroInstance::maxSpellLevel() const
 {
 	return std::min(GameConstants::SPELL_LEVELS, 2 + valOfBonuses(Selector::typeSubtype(Bonus::SECONDARY_SKILL_PREMY, SecondarySkill::WISDOM)));

lib/mapObjects/CGHeroInstance.h

@@ -41,6 +41,11 @@ class DLL_LINKAGE CGHeroInstance : public CArmedInstance, public IBoatGenerator,
 {
 	// We serialize heroes into JSON for crossover
 	friend class CCampaignState;
+	friend class CMapLoaderH3M;
+
+private:
+	std::set<SpellID> spells; //known spells (spell IDs)
+
 public:
 	//////////////////////////////////////////////////////////////////////////
 
@@ -72,7 +77,6 @@ public:
 
 	//std::vector<const CArtifact*> artifacts; //hero's artifacts from bag
 	//std::map<ui16, const CArtifact*> artifWorn; //map<position,artifact_id>; positions: 0 - head; 1 - shoulders; 2 - neck; 3 - right hand; 4 - left hand; 5 - torso; 6 - right ring; 7 - left ring; 8 - feet; 9 - misc1; 10 - misc2; 11 - misc3; 12 - misc4; 13 - mach1; 14 - mach2; 15 - mach3; 16 - mach4; 17 - spellbook; 18 - misc5
-	std::set<SpellID> spells; //known spells (spell IDs)
 	std::set<ObjectInstanceID> visitedObjects;
 
 	struct DLL_LINKAGE Patrol
@@ -148,6 +152,11 @@ public:
 
 	bool hasSpellbook() const;
 	int maxSpellLevel() const;
+	void addSpellToSpellbook(SpellID spell);
+	void removeSpellFromSpellbook(SpellID spell);
+	bool spellbookContainsSpell(SpellID spell) const;
+	void removeSpellbook();
+	const std::set<SpellID> & getSpellsInSpellbook() const;
 	EAlignment::EAlignment getAlignment() const;
 	const std::string &getBiography() const;
 	bool needsLastStack()const override;

lib/mapObjects/MiscObjects.cpp

@@ -1597,7 +1597,7 @@ void CGShrine::onHeroVisit( const CGHeroInstance * h ) const
 	{
 		iw.text.addTxt(MetaString::ADVOB_TXT,131);
 	}
-	else if(vstd::contains(h->spells,spell))//hero already knows the spell
+	else if(h->spellbookContainsSpell(spell))//hero already knows the spell
 	{
 		iw.text.addTxt(MetaString::ADVOB_TXT,174);
 	}
@@ -1649,7 +1649,7 @@ std::string CGShrine::getHoverText(PlayerColor player) const
 std::string CGShrine::getHoverText(const CGHeroInstance * hero) const
 {
 	std::string hoverName = getHoverText(hero->tempOwner);
-	if(wasVisited(hero->tempOwner) && vstd::contains(hero->spells, spell)) //know what spell there is and hero knows that spell
+	if(wasVisited(hero->tempOwner) && hero->spellbookContainsSpell(spell)) //know what spell there is and hero knows that spell
 		hoverName += "\n\n" + VLC->generaltexth->allTexts[354]; // (Already learned)
 	return hoverName;
 }

server/CGameHandler.cpp

@@ -700,7 +700,7 @@ void CGameHandler::endBattle(int3 tile, const CGHeroInstance *hero1, const CGHer
 		{
 			double eagleEyeChance = finishingBattle->winnerHero->valOfBonuses(Bonus::SECONDARY_SKILL_PREMY, SecondarySkill::EAGLE_EYE);
 			for (const CSpell *sp : gs->curB->sides.at(!battleResult.data->winner).usedSpellsHistory)
-				if (sp->level <= eagleEyeLevel && !vstd::contains(finishingBattle->winnerHero->spells, sp->id) && getRandomGenerator().nextInt(99) < eagleEyeChance)
+				if (sp->level <= eagleEyeLevel && !finishingBattle->winnerHero->spellbookContainsSpell(sp->id) && getRandomGenerator().nextInt(99) < eagleEyeChance)
 					cs.spells.insert(sp->id);
 		}
 	}
@@ -2087,7 +2087,7 @@ void CGameHandler::giveSpells(const CGTownInstance *t, const CGHeroInstance *h)
 		{
 			for (int j = 0; j < t->spellsAtLevel(i+1, true) && j < t->spells.at(i).size(); j++)
 			{
-				if (!vstd::contains(h->spells, t->spells.at(i).at(j)))
+				if (!h->spellbookContainsSpell(t->spells.at(i).at(j)))
 					cs.spells.insert(t->spells.at(i).at(j));
 			}
 		}
@@ -2612,16 +2612,16 @@ void CGameHandler::useScholarSkill(ObjectInstanceID fromHero, ObjectInstanceID t
 	ChangeSpells cs1;
 	cs1.learn = true;
 	cs1.hid = toHero;//giving spells to first hero
-	for (auto it : h1->spells)
-		if (h2Lvl >= it.toSpell()->level && !vstd::contains(h2->spells, it))//hero can learn it and don't have it yet
+	for (auto it : h1->getSpellsInSpellbook())
+		if (h2Lvl >= it.toSpell()->level && !h2->spellbookContainsSpell(it))//hero can learn it and don't have it yet
 			cs1.spells.insert(it);//spell to learn
 
 	ChangeSpells cs2;
 	cs2.learn = true;
 	cs2.hid = fromHero;
 
-	for (auto it : h2->spells)
-		if (h1Lvl >= it.toSpell()->level && !vstd::contains(h1->spells, it))
+	for (auto it : h2->getSpellsInSpellbook())
+		if (h1Lvl >= it.toSpell()->level && !h1->spellbookContainsSpell(it))
 			cs2.spells.insert(it);
 
 	if (!cs1.spells.empty() || !cs2.spells.empty())//create a message

test/game/CGameStateTest.cpp

@@ -251,7 +251,7 @@ TEST_F(CGameStateTest, battleResurrection)
 	ASSERT_NE(attacker->tempOwner, defender->tempOwner);
 
 	attacker->setSecSkillLevel(SecondarySkill::EARTH_MAGIC, 3, true);
-	attacker->spells.insert(SpellID::RESURRECTION);
+	attacker->addSpellToSpellbook(SpellID::RESURRECTION);
 	attacker->setPrimarySkill(PrimarySkill::SPELL_POWER, 100, true);
 	attacker->setPrimarySkill(PrimarySkill::KNOWLEDGE, 20, true);
 	attacker->mana = attacker->manaLimit();

test/vcai/ResurceManagerTest.cpp

@@ -13,7 +13,7 @@
 
 #include "../AI/VCAI/VCAI.h"
 #include "ResourceManagerTest.h"
-#include "../AI/VCAI/Goals.h"
+#include "../AI/VCAI/Goals/Goals.h"
 #include "mock_VCAI_CGoal.h"
 #include "mock_VCAI.h"
 #include "mock_ResourceManager.h"