* Probably fixed #655.

* Fixed #736, #737.
* Fixed crash on loss/victory.
* Fixed crash on loading some AB maps.
* Fixed crash on loading map where victory/loss condition objective hero was placed inside the town.
* Fixed crash on loading map when neutral Dungeon has built Portal of Summoning.
* Mutex protecting GS will be used to prevent changes in GS when GUI might read it.
* Little more securities around moving hero and ending turn, still needed more.

CCallback.cpp

@@ -259,7 +259,6 @@ void CCallback::recruitHero(const CGObjectInstance *townOrTavern, const CGHeroIn
 
 bool CCallback::getPath(int3 src, int3 dest, const CGHeroInstance * hero, CPath &ret)
 {
-	boost::shared_lock<boost::shared_mutex> lock(*gs->mx);
 	return gs->getPath(src,dest,hero, ret);
 }
 
@@ -332,6 +331,20 @@ void CCallback::castSpell(const CGHeroInstance *hero, int spellID, const int3 &p
 	cas.pos = pos;
 	sendRequest(&cas);
 }
+
+boost::shared_mutex& CCallback::getGsMutex()
+{
+	return *gs->mx;
+}
+
+void CCallback::unregisterMyInterface()
+{
+	assert(player >= 0); //works only for player callback
+	cl->playerint.erase(player);
+	cl->battleints.erase(player);
+	//TODO? should callback be disabled as well?
+}
+
 CBattleCallback::CBattleCallback(CGameState *GS, int Player, CClient *C )
 {
 	gs = GS;

CCallback.h

@@ -36,6 +36,9 @@ struct CGPathNode;
 struct CGPath;
 struct CPathsInfo;
 
+namespace boost
+{class shared_mutex;}
+
 class IBattleCallback
 {
 public:
@@ -105,13 +108,17 @@ class CCallback : public CPlayerSpecificInfoCallback, public IGameActionCallback
 private:
 	CCallback(CGameState * GS, int Player, CClient *C);
 public:
-//client-specific functionalities (pathfinding)
+	//client-specific functionalities (pathfinding)
 	virtual bool getPath(int3 src, int3 dest, const CGHeroInstance * hero, CPath &ret); //DEPRACATED!!!
 	virtual const CGPathNode *getPathInfo(int3 tile); //uses main, client pathfinder info
 	virtual bool getPath2(int3 dest, CGPath &ret); //uses main, client pathfinder info
 	virtual void calculatePaths(const CGHeroInstance *hero, CPathsInfo &out, int3 src = int3(-1,-1,-1), int movement = -1);
 	virtual void recalculatePaths(); //updates main, client pathfinder info (should be called when moving hero is over)
 
+
+	boost::shared_mutex &getGsMutex(); //just return a reference to mutex, does not lock nor anything
+	void unregisterMyInterface(); //stops delivering information about game events to that player's interface -> can be called ONLY after victory/loss
+
 //commands
 	bool moveHero(const CGHeroInstance *h, int3 dst); //dst must be free, neighbouring tile (this function can move hero only by one tile)
 	bool teleportHero(const CGHeroInstance *who, const CGTownInstance *where);

client/CAdvmapInterface.cpp

@@ -1092,6 +1092,8 @@ void CAdvMapInt::fnextHero()
 
 void CAdvMapInt::fendTurn()
 {
+	if(!LOCPLINT->makingTurn)
+		return;
 	if(LOCPLINT->cingconsole->active)
 		LOCPLINT->cingconsole->deactivate();
 	LOCPLINT->makingTurn = false;

client/CBattleInterface.cpp

@@ -2434,9 +2434,8 @@ void CBattleInterface::newStack(const CStack * stack)
 	creDir[stack->ID] = stack->attackerOwned;
 }
 
-void CBattleInterface::stackRemoved(const CStack * stack)
+void CBattleInterface::stackRemoved(int stackID)
 {
-	int stackID = stack->ID;
 	delete creAnims[stackID];
 	creAnims.erase(stackID);
 	creDir.erase(stackID);

client/CBattleInterface.h

@@ -522,7 +522,7 @@ public:
 	//call-ins
 	void startAction(const BattleAction* action);
 	void newStack(const CStack * stack); //new stack appeared on battlefield
-	void stackRemoved(const CStack * stack); //stack disappeared from batlefiled
+	void stackRemoved(int stackID); //stack disappeared from batlefiled
 	void stackActivated(const CStack * stack); //active stack has been changed
 	void stackMoved(const CStack * stack, THex destHex, bool endMoving, int distance); //stack with id number moved to destHex
 	void waitForAnims();

client/CPlayerInterface.cpp

@@ -650,7 +650,8 @@ void CPlayerInterface::battleStacksRemoved(const BattleStacksRemoved & bsr)
 	boost::unique_lock<boost::recursive_mutex> un(*pim);
 	for(std::set<ui32>::const_iterator it = bsr.stackIDs.begin(); it != bsr.stackIDs.end(); ++it) //for each removed stack
 	{
-		battleInt->stackRemoved(LOCPLINT->cb->battleGetStackByID(*it));
+		battleInt->stackRemoved(*it);
+		//battleInt->stackRemoved(LOCPLINT->cb->battleGetStackByID(*it));
 	}
 }
 
@@ -1045,11 +1046,15 @@ void CPlayerInterface::serialize( CISer<CLoadFile> &h, const int version )
 
 bool CPlayerInterface::moveHero( const CGHeroInstance *h, CGPath path )
 {
+	if(!LOCPLINT->makingTurn)
+		return false;
 	if (!h)
 		return false; //can't find hero
 
+	//evil...
 	eventsM.unlock();
 	pim->unlock();
+	cb->getGsMutex().unlock_shared();
 	bool result = false;
 
 	{
@@ -1106,6 +1111,7 @@ bool CPlayerInterface::moveHero( const CGHeroInstance *h, CGPath path )
 		cb->recalculatePaths();
 	}
 
+	cb->getGsMutex().lock_shared();
 	pim->lock();
 	eventsM.lock();
 	return result;
@@ -1319,6 +1325,9 @@ void CPlayerInterface::update()
 	if(terminate_cond.get())
 		return;
 	
+	//make sure that gamestate won't change when GUI objects may obtain its parts on event processing or drawing request
+	boost::shared_lock<boost::shared_mutex> gsLock(cb->getGsMutex());
+
 	//if there are any waiting dialogs, show them
 	if((howManyPeople <= 1 || makingTurn) && dialogs.size() && !showingDialog->get())
 	{
@@ -1834,6 +1843,7 @@ void CPlayerInterface::gameOver(ui8 player, bool victory )
 			else
 				requestStoppingClient();
 		}
+		cb->unregisterMyInterface(); //we already won/lost, nothing else matters
 
 	}
 	else

client/Client.cpp

@@ -201,6 +201,10 @@ void CClient::endGame( bool closeConnection /*= true*/ )
 {
 	// Game is ending
 	// Tell the network thread to reach a stable state
+	if(closeConnection)
+		stopConnection();
+	tlog0 << "Closed connection." << std::endl;
+
 	GH.curInt = NULL;
 	LOCPLINT->terminate_cond.setn(true);
 	LOCPLINT->pim->lock();
@@ -213,6 +217,7 @@ void CClient::endGame( bool closeConnection /*= true*/ )
 	GH.statusbar = NULL;
 	tlog0 << "Removed GUI." << std::endl;
 
+
 	delete CGI->mh;
 	const_cast<CGameInfo*>(CGI)->mh = NULL;
 
@@ -235,9 +240,6 @@ void CClient::endGame( bool closeConnection /*= true*/ )
 	}
 	tlog0 << "Deleted playerInts." << std::endl;
 
-	if(closeConnection)
-		stopConnection();
-
 	tlog0 << "Client stopped." << std::endl;
 }
 

client/NetPacksClient.cpp

@@ -27,7 +27,15 @@
 #include "../lib/BattleState.h"
 
 
-//macro to avoid code duplication - calls given method with given arguments if interface for specific player is present
+//macros to avoid code duplication - calls given method with given arguments if interface for specific player is present
+//awaiting variadic templates...
+#define CALL_IN_PRIVILAGED_INTS(function, ...)										\
+	do																				\
+	{																				\
+		BOOST_FOREACH(IGameEventsReceiver *ger, cl->privilagedGameEventReceivers)	\
+			ger->function(__VA_ARGS__);												\
+	} while(0)
+
 #define CALL_ONLY_THAT_INTERFACE(player, function, ...)		\
 		do													\
 		{													\
@@ -35,17 +43,14 @@
 			cl->playerint[player]->function(__VA_ARGS__);	\
 		}while(0)											
 
-
-#define INTERFACE_CALL_IF_PRESENT(player,function,...) 		\
-		do													\
-		{													\
+#define INTERFACE_CALL_IF_PRESENT(player,function,...) 				\
+		do															\
+		{															\
 			CALL_ONLY_THAT_INTERFACE(player, function, __VA_ARGS__);\
-			BOOST_FOREACH(IGameEventsReceiver *ger, cl->privilagedGameEventReceivers)\
-				ger->function(__VA_ARGS__);					\
+			CALL_IN_PRIVILAGED_INTS(function, __VA_ARGS__);			\
 		} while(0)										
 
-
-#define CALL_ONLY_THT_BATTLE_INTERFACE(player,function,...) 	\
+#define CALL_ONLY_THT_BATTLE_INTERFACE(player,function, ...) 	\
 	do															\
 	{															\
 		if(vstd::contains(cl->battleints,player))				\
@@ -66,6 +71,16 @@
 		BATTLE_INTERFACE_CALL_RECEIVERS(function, __VA_ARGS__);	\
 	} while(0)
 
+//calls all normal interfaces and privilaged ones, playerints may be updated when iterating over it, so we need a copy
+#define CALL_IN_ALL_INTERFACES(function, ...)							\
+	do																	\
+	{																	\
+		std::map<ui8, CGameInterface*> ints = cl->playerint;			\
+		for(std::map<ui8, CGameInterface*>::iterator i = ints.begin(); i != ints.end(); i++)\
+			CALL_ONLY_THAT_INTERFACE(i->first, function, __VA_ARGS__);	\
+	} while(0)
+
+
 #define BATTLE_INTERFACE_CALL_IF_PRESENT_FOR_BOTH_SIDES(function,...) 				\
 	CALL_ONLY_THT_BATTLE_INTERFACE(GS(cl)->curB->sides[0], function, __VA_ARGS__)	\
 	CALL_ONLY_THT_BATTLE_INTERFACE(GS(cl)->curB->sides[1], function, __VA_ARGS__)	\
@@ -263,12 +278,7 @@ void ChangeObjPos::applyCl( CClient *cl )
 
 void PlayerEndsGame::applyCl( CClient *cl )
 {
-	for(std::map<ui8, CGameInterface*>::iterator i=cl->playerint.begin();i!=cl->playerint.end();i++)
-		i->second->gameOver(player,	victory);
-
-
-// 	if(!CPlayerInterface::howManyPeople)
-// 		cl->terminate = true;
+	CALL_IN_ALL_INTERFACES(gameOver, player, victory);
 }
 
 void RemoveBonus::applyCl( CClient *cl )

lib/CGameState.cpp

@@ -81,8 +81,9 @@ public:
 	void applyOnGS(CGameState *gs, void *pack) const
 	{
 		T *ptr = static_cast<T*>(pack);
-		while(!gs->mx->try_lock())
-			boost::this_thread::sleep(boost::posix_time::milliseconds(50)); //give other threads time to finish
+		gs->mx->lock();
+// 		while(!gs->mx->try_lock())
+// 			boost::this_thread::sleep(boost::posix_time::milliseconds(1)); //give other threads time to finish
 		ptr->applyGs(gs);
 		gs->mx->unlock();
 	}
@@ -1577,6 +1578,9 @@ void CGameState::init( StartInfo * si, ui32 checksum, int Seed )
 			}
 		}
 	}
+
+
+	map->checkForObjectives(); //needs to be run when all objects are properly placed
 }
 
 int CGameState::battleGetBattlefieldType(int3 tile)
@@ -2002,7 +2006,6 @@ bool CGameState::getPath(int3 src, int3 dest, const CGHeroInstance * hero, CPath
 void CGameState::calculatePaths(const CGHeroInstance *hero, CPathsInfo &out, int3 src, int movement)
 {
 	assert(hero);
-	boost::shared_lock<boost::shared_mutex> lock(*mx);
 	if(src.x < 0)
 		src = hero->getPosition(false);
 	if(movement < 0)

lib/CObjectHandler.cpp

@@ -756,7 +756,7 @@ void CGHeroInstance::initHero()
 	else //remove placeholder
 		spells -= 0xffffffff;
 
-	if(!getArt(Arts::MACH4) && type->startingSpell >= 0) //no catapult means we haven't read pre-existant set -> use default rules for spellbook
+	if(!getArt(Arts::MACH4) && !getArt(Arts::SPELLBOOK) && type->startingSpell >= 0) //no catapult means we haven't read pre-existant set -> use default rules for spellbook
 		putArtifact(Arts::SPELLBOOK, CArtifactInstance::createNewArtifactInstance(0));
 
 	if(!getArt(Arts::MACH4))

lib/map.cpp

@@ -417,8 +417,6 @@ void Mapa::initFromBytes(const unsigned char * bufor)
 		addBlockVisTiles(objects[f]);
 	}
 	tlog0<<"\tCalculating blocked/visitable tiles: "<<th.getDif()<<std::endl;
-
-	checkForObjectives();
 	tlog0 << "\tMap initialization done!" << std::endl;
 }	
 void Mapa::removeBlockVisTiles(CGObjectInstance * obj, bool total)
@@ -2005,21 +2003,26 @@ bool Mapa::isWaterTile(const int3 &pos) const
 	return isInTheMap(pos) && getTile(pos).tertype == TerrainTile::water;
 }
 
-void Mapa::checkForObjectives()
+const CGObjectInstance *Mapa::getObjectiveObjectFrom(int3 pos, bool lookForHero)
 {
-	if(isInTheMap(victoryCondition.pos))
+	const std::vector <CGObjectInstance*> &objs = getTile(pos).visitableObjects;
+	assert(objs.size());
+	if(objs.size() > 1 && lookForHero && objs.front()->ID != HEROI_TYPE)
 	{
-		const std::vector <CGObjectInstance*> &objs = getTile(victoryCondition.pos).visitableObjects;
-		assert(objs.size());
-		victoryCondition.obj = objs.front();
+		assert(objs.back()->ID == HEROI_TYPE);
+		return objs.back();
 	}
+	else
+		return objs.front();
+}
+
+void Mapa::checkForObjectives()
+{
+	if(isInTheMap(victoryCondition.pos))
+		victoryCondition.obj = getObjectiveObjectFrom(victoryCondition.pos, victoryCondition.condition == beatHero);
 
 	if(isInTheMap(lossCondition.pos))
-	{
-		const std::vector <CGObjectInstance*> &objs = getTile(lossCondition.pos).visitableObjects;
-		assert(objs.size());
-		lossCondition.obj = objs.front();
-	}
+		lossCondition.obj = getObjectiveObjectFrom(lossCondition.pos, lossCondition.typeOfLossCon == lossHero);
 }
 
 void Mapa::addNewArtifactInstance( CArtifactInstance *art )
@@ -2038,9 +2041,18 @@ bool Mapa::loadArtifactToSlot(CGHeroInstance *h, int slot, const unsigned char *
 	if(isArt)
 	{
 		if(vstd::contains(VLC->arth->bigArtifacts, aid) && slot >= Arts::BACKPACK_START)
+		{
 			tlog3 << "Warning: A big artifact (war machine) in hero's backpack, ignoring...\n";
-		else
-			h->putArtifact(slot, createArt(aid));
+			return false;
+		}
+		if(aid == 0 && slot == Arts::MISC5)
+		{
+			//TODO: check how H3 handles it -> art 0 in slot 18 in AB map
+			tlog3 << "Spellbook to MISC5 slot? Putting it spellbook place. AB format peculiarity ? (format " << (int)version << ")\n";
+			slot = Arts::SPELLBOOK;
+		}
+		
+		h->putArtifact(slot, createArt(aid));
 	}
 	return isArt;
 }

lib/map.h

@@ -329,6 +329,8 @@ struct DLL_EXPORT Mapa : public CMapHeader
 	void addNewArtifactInstance(CArtifactInstance *art);
 	void eraseArtifactInstance(CArtifactInstance *art);
 
+
+	const CGObjectInstance *getObjectiveObjectFrom(int3 pos, bool lookForHero);
 	void checkForObjectives();
 	void addBlockVisTiles(CGObjectInstance * obj);
 	void removeBlockVisTiles(CGObjectInstance * obj, bool total=false);

server/CGameHandler.cpp

@@ -769,6 +769,12 @@ static bool evntCmp(const CMapEvent *a, const CMapEvent *b)
 
 void CGameHandler::setPortalDwelling(const CGTownInstance * town, bool forced=false, bool clear = false)
 {// bool forced = true - if creature should be replaced, if false - only if no creature was set
+	const PlayerState *p = gs->getPlayer(town->tempOwner);
+	if(!p)
+	{
+		tlog3 << "There is no player owner of town " << town->name << " at " << town->pos << std::endl;
+		return;
+	}
 
 	if (forced || town->creatures[CREATURES_PER_TOWN].second.empty())//we need to change creature
 		{
@@ -777,7 +783,7 @@ void CGameHandler::setPortalDwelling(const CGTownInstance * town, bool forced=fa
 			ssi.creatures = town->creatures;
 			ssi.creatures[CREATURES_PER_TOWN].second.clear();//remove old one
 			
-			const std::vector<ConstTransitivePtr<CGDwelling> > &dwellings = gs->getPlayer(town->tempOwner)->dwellings;
+			const std::vector<ConstTransitivePtr<CGDwelling> > &dwellings = p->dwellings;
 			if (dwellings.empty())//no dwellings - just remove
 			{
 				sendAndApply(&ssi);