simplification: remove nullkiller->dangerHitMap->updateHitMap() from RecruitHeroBehavior, BuyArmyBehavior and BuildingBehavior. Now it's called only once before getting into those decomposers;

simplification: remove partialUpdate bool from Nullkiller:updateState as it depends on pathfinderInvalidated anyway;
restructure goal/RecruitHeroBehavior to allow easier testing

AI/Nullkiller2/AIGateway.cpp

@@ -46,13 +46,13 @@ thread_local AIGateway * aiGwTl = nullptr;
 //helper RAII to manage global ai/cb ptrs
 struct SetGlobalState
 {
-	SetGlobalState(AIGateway * gateway)
+	SetGlobalState(AIGateway * aiGw)
 	{
 		assert(!aiGwTl);
 		assert(!ccTl);
 
-		aiGwTl = gateway;
-		ccTl = gateway->cc.get();
+		aiGwTl = aiGw;
+		ccTl = aiGw->cc.get();
 	}
 	~SetGlobalState()
 	{
@@ -64,7 +64,7 @@ struct SetGlobalState
 };
 
 
-#define SET_GLOBAL_STATE(ai) SetGlobalState _hlpSetState(ai)
+#define SET_GLOBAL_STATE(aiGw) SetGlobalState _hlpSetState(aiGw)
 
 #define NET_EVENT_HANDLER SET_GLOBAL_STATE(this)
 #define MAKING_TURN SET_GLOBAL_STATE(this)

AI/Nullkiller2/Behaviors/BuildingBehavior.cpp

@@ -49,7 +49,8 @@ Goals::TGoalVec BuildingBehavior::decompose(const Nullkiller * aiNk) const
 	auto & developmentInfos = aiNk->buildAnalyzer->getDevelopmentInfo();
 	auto isGoldPressureLow = !aiNk->buildAnalyzer->isGoldPressureOverMax();
 
-	aiNk->dangerHitMap->updateHitMap();
+	// Simplification: Moved this call before getting into the decomposer
+	// aiNk->dangerHitMap->updateHitMap();
 
 	for(auto & developmentInfo : developmentInfos)
 	{

AI/Nullkiller2/Behaviors/BuyArmyBehavior.cpp

@@ -35,7 +35,8 @@ Goals::TGoalVec BuyArmyBehavior::decompose(const Nullkiller * aiNk) const
 		return tasks;
 	}
 
-	aiNk->dangerHitMap->updateHitMap();
+	// Simplification: Moved this call before getting into the decomposer
+	// aiNk->dangerHitMap->updateHitMap();
 
 	for(auto town : ccTl->getTownsInfo())
 	{

AI/Nullkiller2/Behaviors/RecruitHeroBehavior.cpp

@@ -27,19 +27,18 @@ std::string RecruitHeroBehavior::toString() const
 Goals::TGoalVec RecruitHeroBehavior::decompose(const Nullkiller * aiNk) const
 {
 	Goals::TGoalVec tasks;
-	auto towns = aiNk->cc->getTownsInfo();
-
-	auto ourHeroes = aiNk->heroManager->getHeroRoles();
+	const auto ourTowns = aiNk->cc->getTownsInfo();
+	const auto ourHeroes = aiNk->heroManager->getHeroRoles();
 	auto minScoreToHireMain = std::numeric_limits<float>::max();
 	int currentArmyValue = 0;
 
-	for(auto hero : ourHeroes)
+	for(const auto & hero : ourHeroes)
 	{
 		currentArmyValue += hero.first->getArmyCost();
 		if(hero.second != HeroRole::MAIN)
 			continue;
 
-		auto newScore = aiNk->heroManager->evaluateHero(hero.first.get());
+		const auto newScore = aiNk->heroManager->evaluateHero(hero.first.get());
 
 		if(minScoreToHireMain > newScore)
 		{
@@ -47,7 +46,8 @@ Goals::TGoalVec RecruitHeroBehavior::decompose(const Nullkiller * aiNk) const
 			minScoreToHireMain = newScore;
 		}
 	}
-	// If we don't have any heros we might want to lower our expectations.
+
+	// If we don't have any heroes, lower our expectations.
 	if (ourHeroes.empty())
 		minScoreToHireMain = 0;
 
@@ -56,76 +56,83 @@ Goals::TGoalVec RecruitHeroBehavior::decompose(const Nullkiller * aiNk) const
 	float bestScore = 0;
 	bool haveCapitol = false;
 
-	aiNk->dangerHitMap->updateHitMap();
+	// Simplification: Moved this call before getting into the decomposer
+	// aiNk->dangerHitMap->updateHitMap();
+
 	int treasureSourcesCount = 0;
 	int bestClosestThreat = UINT8_MAX;
 	
-	for(auto town : towns)
+	for(const auto * town : ourTowns)
 	{
 		uint8_t closestThreat = UINT8_MAX;
-		for (auto threat : aiNk->dangerHitMap->getTownThreats(town))
+		for (const auto & threat : aiNk->dangerHitMap->getTownThreats(town))
 		{
 			closestThreat = std::min(closestThreat, threat.turn);
 		}
-		//Don't hire a hero where there already is one present
+
 		if (town->getVisitingHero() && town->getGarrisonHero())
 			continue;
+
 		float visitability = 0;
-		for (auto checkHero : ourHeroes)
+		for (const auto & hero : ourHeroes)
 		{
-			if (aiNk->dangerHitMap->getClosestTown(checkHero.first.get()->visitablePos()) == town)
+			if (aiNk->dangerHitMap->getClosestTown(hero.first.get()->visitablePos()) == town)
 				visitability++;
 		}
+
 		if(aiNk->heroManager->canRecruitHero(town))
 		{
 			auto availableHeroes = aiNk->cc->getAvailableHeroes(town);
 			
-			for (auto obj : aiNk->objectClusterizer->getNearbyObjects())
+			for (const auto * obj : aiNk->objectClusterizer->getNearbyObjects())
 			{
-				if ((obj->ID == Obj::RESOURCE)
+				if (obj->ID == Obj::RESOURCE
 					|| obj->ID == Obj::TREASURE_CHEST
 					|| obj->ID == Obj::CAMPFIRE
 					|| isWeeklyRevisitable(aiNk->playerID, obj)
 					|| obj->ID == Obj::ARTIFACT)
 				{
 					auto tile = obj->visitablePos();
-					auto closestTown = aiNk->dangerHitMap->getClosestTown(tile);
-
-					if (town == closestTown)
-						treasureSourcesCount++;
+					if (town == aiNk->dangerHitMap->getClosestTown(tile))
+						treasureSourcesCount++; // TODO: Mircea: Shouldn't it be used to determine the best town?
 				}
 			}
 
-			for(auto hero : availableHeroes)
+			for(const auto hero : availableHeroes)
 			{
 				if ((town->getVisitingHero() || town->getGarrisonHero()) 
 					&& closestThreat < 1
 					&& hero->getArmyCost() < GameConstants::HERO_GOLD_COST / 3.0)
 					continue;
+
 				auto score = aiNk->heroManager->evaluateHero(hero);
 				if(score > minScoreToHireMain)
 				{
 					score *= score / minScoreToHireMain;
 				}
-				score *= (hero->getArmyCost() + currentArmyValue);
+				score *= hero->getArmyCost() + currentArmyValue;
+
 				if (hero->getFactionID() == town->getFactionID())
 					score *= 1.5;
 				if (vstd::isAlmostZero(visitability))
 					score *= 30 * town->getTownLevel();
 				else
 					score *= town->getTownLevel() / visitability;
+
 				if (score > bestScore)
 				{
 					bestScore = score;
 					bestHeroToHire = hero;
-					bestTownToHireFrom = town;
+					bestTownToHireFrom = town; // TODO: Mircea: Seems to be no logic to choose the right town?
 					bestClosestThreat = closestThreat;
 				}
 			}
 		}
+
 		if (town->hasCapitol())
 			haveCapitol = true;
 	}
+
 	if (bestHeroToHire && bestTownToHireFrom)
 	{
 		if (aiNk->cc->getHeroesInfo().size() == 0

AI/Nullkiller2/Engine/DeepDecomposer.cpp

@@ -26,8 +26,8 @@ namespace NK2AI
 
 using namespace Goals;
 
-DeepDecomposer::DeepDecomposer(const Nullkiller * ai)
-	:ai(ai), depth(0)
+DeepDecomposer::DeepDecomposer(const Nullkiller * aiNk)
+	:ai(aiNk), depth(0)
 {
 }
 

AI/Nullkiller2/Engine/DeepDecomposer.h

@@ -33,7 +33,7 @@ private:
 	const Nullkiller * ai;
 
 public:
-	DeepDecomposer(const Nullkiller * ai);
+	DeepDecomposer(const Nullkiller * aiNk);
 	void reset();
 	void decompose(Goals::TGoalVec & results, Goals::TSubgoal behavior, int depthLimit);
 

AI/Nullkiller2/Engine/Nullkiller.cpp

@@ -217,7 +217,7 @@ void Nullkiller::decompose(Goals::TGoalVec & results, const Goals::TSubgoal& beh
 {
 	makingTurnInterrupption.interruptionPoint();
 	logAi->debug("Decomposing behavior %s", behavior->toString());
-	auto start = std::chrono::high_resolution_clock::now();
+	const auto start = std::chrono::high_resolution_clock::now();
 	decomposer->decompose(results, behavior, decompositionMaxDepth);
 
 	makingTurnInterrupption.interruptionPoint();
@@ -247,7 +247,7 @@ void Nullkiller::invalidatePathfinderData()
 	pathfinderInvalidated = true;
 }
 
-void Nullkiller::updateState(bool partialUpdate)
+void Nullkiller::updateState()
 {
 	makingTurnInterrupption.interruptionPoint();
 	std::unique_lock lockGuard(aiStateMutex);
@@ -261,8 +261,7 @@ void Nullkiller::updateState(bool partialUpdate)
 
 	if (!pathfinderInvalidated)
 		logAi->trace("Skipping paths regeneration - up to date");
-
-	if(!partialUpdate && pathfinderInvalidated)
+	else
 	{
 		memory->removeInvisibleObjects(cc.get());
 
@@ -502,6 +501,9 @@ bool Nullkiller::makeTurnHelperPriorityPass(Goals::TGoalVec & tempResults, int p
 	{
 		tempResults.clear();
 
+		// Call updateHitMap here instead of inside each of the 3 behaviors
+		dangerHitMap->updateHitMap();
+
 		decompose(tempResults, sptr(RecruitHeroBehavior()), 1);
 		decompose(tempResults, sptr(BuyArmyBehavior()), 1);
 		decompose(tempResults, sptr(BuildingBehavior()), 1);
@@ -516,8 +518,7 @@ bool Nullkiller::makeTurnHelperPriorityPass(Goals::TGoalVec & tempResults, int p
 			if(!executeTask(bestPrioPassTask))
 				return false;
 
-			// TODO: Mircea: Inspect why it's ok to do a partial update if condition is true
-			updateState(bestPrioPassTask->getHero() == nullptr);
+			updateState();
 		}
 		else
 		{

AI/Nullkiller2/Engine/Nullkiller.h

@@ -142,7 +142,7 @@ public:
 
 private:
 	void resetState();
-	void updateState(bool partialUpdate = false);
+	void updateState();
 	void decompose(Goals::TGoalVec & results, const Goals::TSubgoal& behavior, int decompositionMaxDepth) const;
 	Goals::TTask choseBestTask(Goals::TGoalVec & tasks) const;
 	Goals::TTaskVec buildPlan(Goals::TGoalVec & tasks, int priorityTier) const;

AI/Nullkiller2/Goals/Composition.cpp

@@ -59,7 +59,7 @@ void Composition::accept(AIGateway * aiGw)
 	}
 }
 
-TGoalVec Composition::decompose(const Nullkiller * ai) const
+TGoalVec Composition::decompose(const Nullkiller * aiNk) const
 {
 	TGoalVec result;
 

AI/Nullkiller2/Goals/Composition.h

@@ -32,7 +32,7 @@ namespace Goals
 		Composition & addNext(const AbstractGoal & goal);
 		Composition & addNext(TSubgoal goal);
 		Composition & addNextSequence(const TGoalVec & taskSequence);
-		TGoalVec decompose(const Nullkiller * ai) const override;
+		TGoalVec decompose(const Nullkiller * aiNk) const override;
 		bool isElementar() const override;
 		int getHeroExchangeCount() const override;