Merge pull request #2850 from IvanSavenko/remove_hero_move_thread

Remove hero movement thread

AI/EmptyAI/CEmptyAI.cpp

@@ -61,7 +61,7 @@ void CEmptyAI::showBlockingDialog(const std::string &text, const std::vector<Com
 	cb->selectionMade(0, askID);
 }
 
-void CEmptyAI::showTeleportDialog(TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID)
+void CEmptyAI::showTeleportDialog(const CGHeroInstance * hero, TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID)
 {
 	cb->selectionMade(0, askID);
 }

AI/EmptyAI/CEmptyAI.h

@@ -29,7 +29,7 @@ public:
 	void heroGotLevel(const CGHeroInstance *hero, PrimarySkill pskill, std::vector<SecondarySkill> &skills, QueryID queryID) override;
 	void commanderGotLevel (const CCommanderInstance * commander, std::vector<ui32> skills, QueryID queryID) override;
 	void showBlockingDialog(const std::string &text, const std::vector<Component> &components, QueryID askID, const int soundID, bool selection, bool cancel) override;
-	void showTeleportDialog(TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID) override;
+	void showTeleportDialog(const CGHeroInstance * hero, TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID) override;
 	void showGarrisonDialog(const CArmedInstance *up, const CGHeroInstance *down, bool removableUnits, QueryID queryID) override;
 	void showMapObjectSelectDialog(QueryID askID, const Component & icon, const MetaString & title, const MetaString & description, const std::vector<ObjectInstanceID> & objects) override;
 	std::optional<BattleAction> makeSurrenderRetreatDecision(const BattleID & battleID, const BattleStateInfoForRetreat & battleState) override;

AI/Nullkiller/AIGateway.cpp

@@ -654,7 +654,7 @@ void AIGateway::showBlockingDialog(const std::string & text, const std::vector<C
 	});
 }
 
-void AIGateway::showTeleportDialog(TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID)
+void AIGateway::showTeleportDialog(const CGHeroInstance * hero, TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID)
 {
 	NET_EVENT_HANDLER;
 	status.addQuery(askID, boost::str(boost::format("Teleport dialog query with %d exits") % exits.size()));

AI/Nullkiller/AIGateway.h

@@ -117,7 +117,7 @@ public:
 	void commanderGotLevel(const CCommanderInstance * commander, std::vector<ui32> skills, QueryID queryID) override; //TODO
 	void showBlockingDialog(const std::string & text, const std::vector<Component> & components, QueryID askID, const int soundID, bool selection, bool cancel) override; //Show a dialog, player must take decision. If selection then he has to choose between one of given components, if cancel he is allowed to not choose. After making choice, CCallback::selectionMade should be called with number of selected component (1 - n) or 0 for cancel (if allowed) and askID.
 	void showGarrisonDialog(const CArmedInstance * up, const CGHeroInstance * down, bool removableUnits, QueryID queryID) override; //all stacks operations between these objects become allowed, interface has to call onEnd when done
-	void showTeleportDialog(TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID) override;
+	void showTeleportDialog(const CGHeroInstance * hero, TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID) override;
 	void showMapObjectSelectDialog(QueryID askID, const Component & icon, const MetaString & title, const MetaString & description, const std::vector<ObjectInstanceID> & objects) override;
 	void saveGame(BinarySerializer & h, const int version) override; //saving
 	void loadGame(BinaryDeserializer & h, const int version) override; //loading

AI/VCAI/VCAI.cpp

@@ -656,7 +656,7 @@ void VCAI::showBlockingDialog(const std::string & text, const std::vector<Compon
 	});
 }
 
-void VCAI::showTeleportDialog(TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID)
+void VCAI::showTeleportDialog(const CGHeroInstance * hero, TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID)
 {
 //	LOG_TRACE_PARAMS(logAi, "askID '%i', exits '%s'", askID % exits);
 	NET_EVENT_HANDLER;

AI/VCAI/VCAI.h

@@ -150,7 +150,7 @@ public:
 	void commanderGotLevel(const CCommanderInstance * commander, std::vector<ui32> skills, QueryID queryID) override; //TODO
 	void showBlockingDialog(const std::string & text, const std::vector<Component> & components, QueryID askID, const int soundID, bool selection, bool cancel) override; //Show a dialog, player must take decision. If selection then he has to choose between one of given components, if cancel he is allowed to not choose. After making choice, CCallback::selectionMade should be called with number of selected component (1 - n) or 0 for cancel (if allowed) and askID.
 	void showGarrisonDialog(const CArmedInstance * up, const CGHeroInstance * down, bool removableUnits, QueryID queryID) override; //all stacks operations between these objects become allowed, interface has to call onEnd when done
-	void showTeleportDialog(TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID) override;
+	void showTeleportDialog(const CGHeroInstance * hero, TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID) override;
 	void showMapObjectSelectDialog(QueryID askID, const Component & icon, const MetaString & title, const MetaString & description, const std::vector<ObjectInstanceID> & objects) override;
 	void saveGame(BinarySerializer & h, const int version) override; //saving
 	void loadGame(BinaryDeserializer & h, const int version) override; //loading

client/CMakeLists.txt

@@ -142,6 +142,7 @@ set(client_SRCS
 	CVideoHandler.cpp
 	Client.cpp
 	ClientCommandManager.cpp
+	HeroMovementController.cpp
 	NetPacksClient.cpp
 	NetPacksLobbyClient.cpp
 )
@@ -304,6 +305,7 @@ set(client_HEADERS
 	Client.h
 	ClientCommandManager.h
 	ClientNetPackVisitors.h
+	HeroMovementController.h
 	LobbyClientNetPackVisitors.h
 	resource.h
 )

client/CPlayerInterface.cpp

@@ -12,104 +12,112 @@
 
 #include <vcmi/Artifact.h>
 
+#include "CGameInfo.h"
+#include "CMT.h"
+#include "CMusicHandler.h"
+#include "CServerHandler.h"
+#include "HeroMovementController.h"
+#include "PlayerLocalState.h"
+
 #include "adventureMap/AdventureMapInterface.h"
-#include "mapView/mapHandler.h"
+#include "adventureMap/CInGameConsole.h"
 #include "adventureMap/CList.h"
-#include "battle/BattleInterface.h"
+
 #include "battle/BattleEffectsController.h"
 #include "battle/BattleFieldController.h"
+#include "battle/BattleInterface.h"
 #include "battle/BattleInterfaceClasses.h"
 #include "battle/BattleWindow.h"
-#include "../CCallback.h"
+
-#include "windows/CCastleInterface.h"
 #include "eventsSDL/InputHandler.h"
-#include "mainmenu/CMainMenu.h"
+#include "eventsSDL/NotificationHandler.h"
+
+#include "gui/CGuiHandler.h"
 #include "gui/CursorHandler.h"
-#include "windows/CKingdomInterface.h"
+#include "gui/WindowHandler.h"
-#include "CGameInfo.h"
+
-#include "PlayerLocalState.h"
+#include "mainmenu/CMainMenu.h"
-#include "CMT.h"
+
-#include "windows/CHeroWindow.h"
+#include "mapView/mapHandler.h"
-#include "windows/CCreatureWindow.h"
+
-#include "windows/CQuestLog.h"
-#include "windows/CPuzzleWindow.h"
-#include "widgets/CComponent.h"
-#include "widgets/CGarrisonInt.h"
-#include "widgets/Buttons.h"
-#include "windows/CTradeWindow.h"
-#include "windows/CSpellWindow.h"
-#include "../lib/CConfigHandler.h"
-#include "windows/GUIClasses.h"
 #include "render/CAnimation.h"
 #include "render/IImage.h"
+
+#include "widgets/Buttons.h"
+#include "widgets/CComponent.h"
+#include "widgets/CGarrisonInt.h"
+
+#include "windows/CCastleInterface.h"
+#include "windows/CCreatureWindow.h"
+#include "windows/CHeroWindow.h"
+#include "windows/CKingdomInterface.h"
+#include "windows/CPuzzleWindow.h"
+#include "windows/CQuestLog.h"
+#include "windows/CSpellWindow.h"
+#include "windows/CTradeWindow.h"
+#include "windows/GUIClasses.h"
+#include "windows/InfoWindows.h"
+
+#include "../CCallback.h"
+
 #include "../lib/CArtHandler.h"
+#include "../lib/CConfigHandler.h"
 #include "../lib/CGeneralTextHandler.h"
 #include "../lib/CHeroHandler.h"
+#include "../lib/CPlayerState.h"
+#include "../lib/CStack.h"
+#include "../lib/CStopWatch.h"
+#include "../lib/CThreadHelper.h"
+#include "../lib/CTownHandler.h"
+#include "../lib/CondSh.h"
+#include "../lib/GameConstants.h"
+#include "../lib/JsonNode.h"
+#include "../lib/NetPacks.h" //todo: remove
+#include "../lib/NetPacksBase.h"
+#include "../lib/RoadHandler.h"
+#include "../lib/StartInfo.h"
+#include "../lib/TerrainHandler.h"
+#include "../lib/TextOperations.h"
+#include "../lib/UnlockGuard.h"
+#include "../lib/VCMIDirs.h"
+
 #include "../lib/bonuses/CBonusSystemNode.h"
 #include "../lib/bonuses/Limiters.h"
-#include "../lib/bonuses/Updaters.h"
 #include "../lib/bonuses/Propagators.h"
-#include "../lib/serializer/CTypeList.h"
+#include "../lib/bonuses/Updaters.h"
-#include "../lib/serializer/BinaryDeserializer.h"
+
-#include "../lib/serializer/BinarySerializer.h"
+#include "../lib/gameState/CGameState.h"
-#include "../lib/spells/CSpellHandler.h"
+
-#include "../lib/CTownHandler.h"
 #include "../lib/mapObjects/CGTownInstance.h"
 #include "../lib/mapObjects/MiscObjects.h"
 #include "../lib/mapObjects/ObjectTemplate.h"
+
 #include "../lib/mapping/CMapHeader.h"
+
 #include "../lib/pathfinder/CGPathNode.h"
-#include "../lib/CStack.h"
+
-#include "../lib/JsonNode.h"
+#include "../lib/serializer/BinaryDeserializer.h"
-#include "CMusicHandler.h"
+#include "../lib/serializer/BinarySerializer.h"
-#include "../lib/CondSh.h"
+#include "../lib/serializer/CTypeList.h"
-#include "../lib/NetPacksBase.h"
+
-#include "../lib/NetPacks.h"//todo: remove
+#include "../lib/spells/CSpellHandler.h"
-#include "../lib/VCMIDirs.h"
-#include "../lib/CStopWatch.h"
-#include "../lib/StartInfo.h"
-#include "../lib/TextOperations.h"
-#include "../lib/CPlayerState.h"
-#include "../lib/GameConstants.h"
-#include "gui/CGuiHandler.h"
-#include "gui/WindowHandler.h"
-#include "windows/InfoWindows.h"
-#include "../lib/UnlockGuard.h"
-#include "../lib/RoadHandler.h"
-#include "../lib/TerrainHandler.h"
-#include "../lib/CThreadHelper.h"
-#include "CServerHandler.h"
-// FIXME: only needed for CGameState::mutex
-#include "../lib/gameState/CGameState.h"
-#include "eventsSDL/NotificationHandler.h"
-#include "adventureMap/CInGameConsole.h"
 
 // The macro below is used to mark functions that are called by client when game state changes.
 // They all assume that CPlayerInterface::pim mutex is locked.
 #define EVENT_HANDLER_CALLED_BY_CLIENT
 
-// The macro marks functions that are run on a new thread by client.
+#define BATTLE_EVENT_POSSIBLE_RETURN	\
-// They do not own any mutexes intiially.
+	if (LOCPLINT != this)				\
-#define THREAD_CREATED_BY_CLIENT
+		return;							\
-
-#define RETURN_IF_QUICK_COMBAT		\
 	if (isAutoFightOn && !battleInt)	\
 		return;
 
-#define BATTLE_EVENT_POSSIBLE_RETURN\
-	if (LOCPLINT != this)			\
-		return;						\
-	RETURN_IF_QUICK_COMBAT
-
 boost::recursive_mutex * CPlayerInterface::pim = new boost::recursive_mutex;
 
 CPlayerInterface * LOCPLINT;
 
 std::shared_ptr<BattleInterface> CPlayerInterface::battleInt;
 
-enum  EMoveState {STOP_MOVE, WAITING_MOVE, CONTINUE_MOVE, DURING_MOVE};
-CondSh<EMoveState> stillMoveHero(STOP_MOVE); //used during hero movement
-
 struct HeroObjectRetriever
 {
 	const CGHeroInstance * operator()(const ConstTransitivePtr<CGHeroInstance> &h) const
@@ -123,11 +131,10 @@ struct HeroObjectRetriever
 };
 
 CPlayerInterface::CPlayerInterface(PlayerColor Player):
-	localState(std::make_unique<PlayerLocalState>(*this))
+	localState(std::make_unique<PlayerLocalState>(*this)),
+	movementController(std::make_unique<HeroMovementController>())
 {
 	logGlobal->trace("\tHuman player interface for player %s being constructed", Player.toString());
-	destinationTeleport = ObjectInstanceID();
-	destinationTeleportPos = int3(-1);
 	GH.defActionsDef = 0;
 	LOCPLINT = this;
 	playerID=Player;
@@ -140,7 +147,6 @@ CPlayerInterface::CPlayerInterface(PlayerColor Player):
 	firstCall = 1; //if loading will be overwritten in serialize
 	autosaveCount = 0;
 	isAutoFightOn = false;
-	duringMovement = false;
 	ignoreEvents = false;
 	numOfMovedArts = 0;
 }
@@ -171,7 +177,7 @@ void CPlayerInterface::playerStartsTurn(PlayerColor player)
 	EVENT_HANDLER_CALLED_BY_CLIENT;
 
 	makingTurn = false;
-	stillMoveHero.setn(STOP_MOVE);
+	movementController->onPlayerTurnStarted();
 
 	if(GH.windows().findWindows<AdventureMapInterface>().empty())
 	{
@@ -341,90 +347,7 @@ void CPlayerInterface::heroMoved(const TryMoveHero & details, bool verbose)
 	if (!hero)
 		return;
 
-	if (details.result == TryMoveHero::EMBARK || details.result == TryMoveHero::DISEMBARK)
+	movementController->onTryMoveHero(hero, details);
-	{
-		if(hero->getRemovalSound() && hero->tempOwner == playerID)
-			CCS->soundh->playSound(hero->getRemovalSound().value());
-	}
-
-	std::unordered_set<int3> changedTiles {
-		hero->convertToVisitablePos(details.start),
-		hero->convertToVisitablePos(details.end)
-	};
-	adventureInt->onMapTilesChanged(changedTiles);
-	adventureInt->onHeroMovementStarted(hero);
-
-	bool directlyAttackingCreature = details.attackedFrom && localState->hasPath(hero) && localState->getPath(hero).endPos() == *details.attackedFrom;
-
-	if(makingTurn && hero->tempOwner == playerID) //we are moving our hero - we may need to update assigned path
-	{
-		if(details.result == TryMoveHero::TELEPORTATION)
-		{
-			if(localState->hasPath(hero))
-			{
-				assert(localState->getPath(hero).nodes.size() >= 2);
-				auto nodesIt = localState->getPath(hero).nodes.end() - 1;
-
-				if((nodesIt)->coord == hero->convertToVisitablePos(details.start)
-					&& (nodesIt - 1)->coord == hero->convertToVisitablePos(details.end))
-				{
-					//path was between entrance and exit of teleport -> OK, erase node as usual
-					localState->removeLastNode(hero);
-				}
-				else
-				{
-					//teleport was not along current path, it'll now be invalid (hero is somewhere else)
-					localState->erasePath(hero);
-
-				}
-			}
-		}
-
-		if(hero->pos != details.end //hero didn't change tile but visit succeeded
-			|| directlyAttackingCreature) // or creature was attacked from endangering tile.
-		{
-			localState->erasePath(hero);
-		}
-		else if(localState->hasPath(hero) && hero->pos == details.end) //&& hero is moving
-		{
-			if(details.start != details.end) //so we don't touch path when revisiting with spacebar
-				localState->removeLastNode(hero);
-		}
-	}
-
-	if(details.stopMovement()) //hero failed to move
-	{
-		stillMoveHero.setn(STOP_MOVE);
-		adventureInt->onHeroChanged(hero);
-		return;
-	}
-
-	CGI->mh->waitForOngoingAnimations();
-
-	//move finished
-	adventureInt->onHeroChanged(hero);
-
-	//check if user cancelled movement
-	{
-		if (GH.input().ignoreEventsUntilInput())
-			stillMoveHero.setn(STOP_MOVE);
-	}
-
-	if (stillMoveHero.get() == WAITING_MOVE)
-		stillMoveHero.setn(DURING_MOVE);
-
-	// Hero attacked creature directly, set direction to face it.
-	if (directlyAttackingCreature) {
-		// Get direction to attacker.
-		int3 posOffset = *details.attackedFrom - details.end + int3(2, 1, 0);
-		static const ui8 dirLookup[3][3] = {
-			{ 1, 2, 3 },
-			{ 8, 0, 4 },
-			{ 7, 6, 5 }
-		};
-		// FIXME: Avoid const_cast, make moveDir mutable in some other way?
-		const_cast<CGHeroInstance *>(hero)->moveDir = dirLookup[posOffset.y][posOffset.x];
-	}
 }
 
 void CPlayerInterface::heroKilled(const CGHeroInstance* hero)
@@ -660,10 +583,7 @@ void CPlayerInterface::buildChanged(const CGTownInstance *town, BuildingID build
 
 void CPlayerInterface::battleStartBefore(const BattleID & battleID, const CCreatureSet *army1, const CCreatureSet *army2, int3 tile, const CGHeroInstance *hero1, const CGHeroInstance *hero2)
 {
-	// when battle starts, game will send battleStart pack *before* movement confirmation
+	movementController->onBattleStarted();
-	// and since network thread wait for battle intro to play, movement confirmation will only happen after intro
-	// leading to several bugs, such as blocked input during intro
-	stillMoveHero.setn(STOP_MOVE);
 
 	//Don't wait for dialogs when we are non-active hot-seat player
 	if (LOCPLINT == this)
@@ -914,7 +834,6 @@ void CPlayerInterface::battleTriggerEffect(const BattleID & battleID, const Batt
 	EVENT_HANDLER_CALLED_BY_CLIENT;
 	BATTLE_EVENT_POSSIBLE_RETURN;
 
-	RETURN_IF_QUICK_COMBAT;
 	battleInt->effectsController->battleTriggerEffect(bte);
 
 	if(bte.effect == vstd::to_underlying(BonusType::MANA_DRAIN))
@@ -1017,6 +936,9 @@ void CPlayerInterface::showInfoDialog(EInfoWindowMode type, const std::string &t
 		waitWhileDialog(); //Fix for mantis #98
 		adventureInt->showInfoBoxMessage(components, text, timer);
 
+		// abort movement, if any. Strictly speaking unnecessary, but prevents some edge cases, like movement sound on visiting Magi Hut with "show messages in status window" on
+		movementController->requestMovementAbort();
+
 		if (makingTurn && GH.windows().count() > 0 && LOCPLINT == this)
 			CCS->soundh->playSound(static_cast<soundBase::soundID>(soundID));
 		return;
@@ -1062,7 +984,7 @@ void CPlayerInterface::showInfoDialog(const std::string &text, const std::vector
 	{
 		CCS->soundh->playSound(static_cast<soundBase::soundID>(soundID));
 		showingDialog->set(true);
-		stopMovement(); // interrupt movement to show dialog
+		movementController->requestMovementAbort(); // interrupt movement to show dialog
 		GH.windows().pushWindow(temp);
 	}
 	else
@@ -1085,7 +1007,7 @@ void CPlayerInterface::showYesNoDialog(const std::string &text, CFunctionList<vo
 {
 	boost::unique_lock<boost::recursive_mutex> un(*pim);
 
-	stopMovement();
+	movementController->requestMovementAbort();
 	LOCPLINT->showingDialog->setn(true);
 	CInfoWindow::showYesNoDialog(text, components, onYes, onNo, playerID);
 }
@@ -1095,7 +1017,7 @@ void CPlayerInterface::showBlockingDialog( const std::string &text, const std::v
 	EVENT_HANDLER_CALLED_BY_CLIENT;
 	waitWhileDialog();
 
-	stopMovement();
+	movementController->requestMovementAbort();
 	CCS->soundh->playSound(static_cast<soundBase::soundID>(soundID));
 
 	if (!selection && cancel) //simple yes/no dialog
@@ -1128,15 +1050,10 @@ void CPlayerInterface::showBlockingDialog( const std::string &text, const std::v
 	}
 }
 
-void CPlayerInterface::showTeleportDialog(TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID)
+void CPlayerInterface::showTeleportDialog(const CGHeroInstance * hero, TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID)
 {
 	EVENT_HANDLER_CALLED_BY_CLIENT;
-	int choosenExit = -1;
+	movementController->showTeleportDialog(hero, channel, exits, impassable, askID);
-	auto neededExit = std::make_pair(destinationTeleport, destinationTeleportPos);
-	if (destinationTeleport != ObjectInstanceID() && vstd::contains(exits, neededExit))
-		choosenExit = vstd::find_pos(exits, neededExit);
-
-	cb->selectionMade(choosenExit, askID);
 }
 
 void CPlayerInterface::showMapObjectSelectDialog(QueryID askID, const Component & icon, const MetaString & title, const MetaString & description, const std::vector<ObjectInstanceID> & objects)
@@ -1247,6 +1164,11 @@ void CPlayerInterface::loadGame( BinaryDeserializer & h, const int version )
 
 void CPlayerInterface::moveHero( const CGHeroInstance *h, const CGPath& path )
 {
+	assert(LOCPLINT->makingTurn);
+	assert(h);
+	assert(!showingDialog->get());
+	assert(dialogs.empty());
+
 	LOG_TRACE(logGlobal);
 	if (!LOCPLINT->makingTurn)
 		return;
@@ -1257,12 +1179,10 @@ void CPlayerInterface::moveHero( const CGHeroInstance *h, const CGPath& path )
 	if (showingDialog->get() || !dialogs.empty())
 		return;
 
-	setMovementStatus(true);
-
 	if (localState->isHeroSleeping(h))
 		localState->setHeroAwaken(h);
 
-	boost::thread moveHeroTask(std::bind(&CPlayerInterface::doMoveHero,this,h,path));
+	movementController->requestMovementStart(h, path);
 }
 
 void CPlayerInterface::showGarrisonDialog( const CArmedInstance *up, const CGHeroInstance *down, bool removableUnits, QueryID queryID)
@@ -1270,7 +1190,7 @@ void CPlayerInterface::showGarrisonDialog( const CArmedInstance *up, const CGHer
 	EVENT_HANDLER_CALLED_BY_CLIENT;
 	auto onEnd = [=](){ cb->selectionMade(0, queryID); };
 
-	if (stillMoveHero.get() == DURING_MOVE  && localState->hasPath(down) && localState->getPath(down).nodes.size() > 1) //to ignore calls on passing through garrisons
+	if (movementController->isHeroMovingThroughGarrison(down, up))
 	{
 		onEnd();
 		return;
@@ -1314,22 +1234,13 @@ void CPlayerInterface::showArtifactAssemblyDialog(const Artifact * artifact, con
 
 void CPlayerInterface::requestRealized( PackageApplied *pa )
 {
-	EVENT_HANDLER_CALLED_BY_CLIENT;
+	if(pa->packType == typeList.getTypeID<MoveHero>())
-	if (pa->packType == typeList.getTypeID<MoveHero>()  &&  stillMoveHero.get() == DURING_MOVE
+		movementController->onMoveHeroApplied();
-	   && destinationTeleport == ObjectInstanceID())
-		stillMoveHero.setn(CONTINUE_MOVE);
 
-	if (destinationTeleport != ObjectInstanceID()
+	if(pa->packType == typeList.getTypeID<QueryReply>())
-	   && pa->packType == typeList.getTypeID<QueryReply>()
+		movementController->onQueryReplyApplied();
-	   && stillMoveHero.get() == DURING_MOVE)
-	{ // After teleportation via CGTeleport object is finished
-		destinationTeleport = ObjectInstanceID();
-		destinationTeleportPos = int3(-1);
-		stillMoveHero.setn(CONTINUE_MOVE);
-	}
 }
 
-
 void CPlayerInterface::showHeroExchange(ObjectInstanceID hero1, ObjectInstanceID hero2)
 {
 	heroExchangeStarted(hero1, hero2, QueryID(-1));
@@ -1712,12 +1623,6 @@ void CPlayerInterface::battleNewRoundFirst(const BattleID & battleID)
 	battleInt->newRoundFirst();
 }
 
-void CPlayerInterface::stopMovement()
-{
-	if (stillMoveHero.get() == DURING_MOVE)//if we are in the middle of hero movement
-		stillMoveHero.setn(STOP_MOVE); //after showing dialog movement will be stopped
-}
-
 void CPlayerInterface::showMarketWindow(const IMarket *market, const CGHeroInstance *visitor)
 {
 	EVENT_HANDLER_CALLED_BY_CLIENT;
@@ -1910,7 +1815,7 @@ void CPlayerInterface::proposeLoadingGame()
 
 bool CPlayerInterface::capturedAllEvents()
 {
-	if(duringMovement)
+	if(movementController->isHeroMoving())
 	{
 		//just inform that we are capturing events. they will be processed by heroMoved() in client thread.
 		return true;
@@ -1928,197 +1833,6 @@ bool CPlayerInterface::capturedAllEvents()
 	return false;
 }
 
-void CPlayerInterface::setMovementStatus(bool value)
-{
-	duringMovement = value;
-	if (value)
-	{
-		CCS->curh->hide();
-	}
-	else
-	{
-		CCS->curh->show();
-	}
-}
-
-void CPlayerInterface::doMoveHero(const CGHeroInstance * h, CGPath path)
-{
-	setThreadName("doMoveHero");
-
-	int i = 1;
-	auto getObj = [&](int3 coord, bool ignoreHero)
-	{
-		return cb->getTile(h->convertToVisitablePos(coord))->topVisitableObj(ignoreHero);
-	};
-
-	auto isTeleportAction = [&](EPathNodeAction action) -> bool
-	{
-		if (action != EPathNodeAction::TELEPORT_NORMAL &&
-			action != EPathNodeAction::TELEPORT_BLOCKING_VISIT &&
-			action != EPathNodeAction::TELEPORT_BATTLE)
-		{
-			return false;
-		}
-
-		return true;
-	};
-
-	auto getDestTeleportObj = [&](const CGObjectInstance * currentObject, const CGObjectInstance * nextObjectTop, const CGObjectInstance * nextObject) -> const CGObjectInstance *
-	{
-		if (CGTeleport::isConnected(currentObject, nextObjectTop))
-			return nextObjectTop;
-		if (nextObjectTop && nextObjectTop->ID == Obj::HERO &&
-			CGTeleport::isConnected(currentObject, nextObject))
-		{
-			return nextObject;
-		}
-
-		return nullptr;
-	};
-
-	boost::unique_lock<boost::mutex> un(stillMoveHero.mx);
-	stillMoveHero.data = CONTINUE_MOVE;
-	auto doMovement = [&](int3 dst, bool transit)
-	{
-		stillMoveHero.data = WAITING_MOVE;
-		cb->moveHero(h, dst, transit);
-		while(stillMoveHero.data != STOP_MOVE && stillMoveHero.data != CONTINUE_MOVE)
-			stillMoveHero.cond.wait(un);
-	};
-
-	{
-		for (auto & elem : path.nodes)
-			elem.coord = h->convertFromVisitablePos(elem.coord);
-
-		int soundChannel = -1;
-		AudioPath soundName;
-
-		auto getMovementSoundFor = [&](const CGHeroInstance * hero, int3 posPrev, int3 posNext, EPathNodeAction moveType) -> AudioPath
-		{
-			if (moveType == EPathNodeAction::TELEPORT_BATTLE || moveType == EPathNodeAction::TELEPORT_BLOCKING_VISIT || moveType == EPathNodeAction::TELEPORT_NORMAL)
-				return {};
-
-			if (moveType == EPathNodeAction::EMBARK || moveType == EPathNodeAction::DISEMBARK)
-				return {};
-
-			if (moveType == EPathNodeAction::BLOCKING_VISIT)
-				return {};
-
-			// flying movement sound
-			if (hero->hasBonusOfType(BonusType::FLYING_MOVEMENT))
-				return AudioPath::builtin("HORSE10.wav");
-
-			auto prevTile = cb->getTile(h->convertToVisitablePos(posPrev));
-			auto nextTile = cb->getTile(h->convertToVisitablePos(posNext));
-
-			auto prevRoad = prevTile->roadType;
-			auto nextRoad = nextTile->roadType;
-			bool movingOnRoad = prevRoad->getId() != Road::NO_ROAD && nextRoad->getId() != Road::NO_ROAD;
-
-			if (movingOnRoad)
-				return nextTile->terType->horseSound;
-			else
-				return nextTile->terType->horseSoundPenalty;
-		};
-
-		auto canStop = [&](CGPathNode * node) -> bool
-		{
-			if (node->layer != EPathfindingLayer::LAND && node->layer != EPathfindingLayer::SAIL)
-				return false;
-
-			if (node->accessible != EPathAccessibility::ACCESSIBLE)
-				return false;
-
-			return true;
-		};
-
-		for (i=(int)path.nodes.size()-1; i>0 && (stillMoveHero.data == CONTINUE_MOVE || !canStop(&path.nodes[i])); i--)
-		{
-			int3 prevCoord = path.nodes[i].coord;
-			int3 nextCoord = path.nodes[i-1].coord;
-
-			auto prevObject = getObj(prevCoord, prevCoord == h->pos);
-			auto nextObjectTop = getObj(nextCoord, false);
-			auto nextObject = getObj(nextCoord, true);
-			auto destTeleportObj = getDestTeleportObj(prevObject, nextObjectTop, nextObject);
-			if (isTeleportAction(path.nodes[i-1].action) && destTeleportObj != nullptr)
-			{
-				CCS->soundh->stopSound(soundChannel);
-				destinationTeleport = destTeleportObj->id;
-				destinationTeleportPos = nextCoord;
-				doMovement(h->pos, false);
-				if (path.nodes[i-1].action == EPathNodeAction::TELEPORT_BLOCKING_VISIT
-					|| path.nodes[i-1].action == EPathNodeAction::TELEPORT_BATTLE)
-				{
-					destinationTeleport = ObjectInstanceID();
-					destinationTeleportPos = int3(-1);
-				}
-				if(i != path.nodes.size() - 1)
-				{
-					soundName = getMovementSoundFor(h, prevCoord, nextCoord, path.nodes[i-1].action);
-					if (!soundName.empty())
-						soundChannel = CCS->soundh->playSound(soundName, -1);
-					else
-						soundChannel = -1;
-				}
-				continue;
-			}
-
-			if (path.nodes[i-1].turns)
-			{ //stop sending move requests if the next node can't be reached at the current turn (hero exhausted his move points)
-				stillMoveHero.data = STOP_MOVE;
-				break;
-			}
-
-			{
-				// Start a new sound for the hero movement or let the existing one carry on.
-				AudioPath newSoundName = getMovementSoundFor(h, prevCoord, nextCoord, path.nodes[i-1].action);
-
-				if(newSoundName != soundName)
-				{
-					soundName = newSoundName;
-
-					CCS->soundh->stopSound(soundChannel);
-					if (!soundName.empty())
-						soundChannel = CCS->soundh->playSound(soundName, -1);
-					else
-						soundChannel = -1;
-				}
-			}
-
-			assert(h->pos.z == nextCoord.z); // Z should change only if it's movement via teleporter and in this case this code shouldn't be executed at all
-			int3 endpos(nextCoord.x, nextCoord.y, h->pos.z);
-			logGlobal->trace("Requesting hero movement to %s", endpos.toString());
-
-			bool useTransit = false;
-			if ((i-2 >= 0) // Check there is node after next one; otherwise transit is pointless
-				&& (CGTeleport::isConnected(nextObjectTop, getObj(path.nodes[i-2].coord, false))
-					|| CGTeleport::isTeleport(nextObjectTop)))
-			{ // Hero should be able to go through object if it's allow transit
-				useTransit = true;
-			}
-			else if (path.nodes[i-1].layer == EPathfindingLayer::AIR)
-				useTransit = true;
-
-			doMovement(endpos, useTransit);
-
-			logGlobal->trace("Resuming %s", __FUNCTION__);
-			bool guarded = cb->isInTheMap(cb->getGuardingCreaturePosition(endpos - int3(1, 0, 0)));
-			if ((!useTransit && guarded) || showingDialog->get() == true) // Abort movement if a guard was fought or there is a dialog to display (Mantis #1136)
-				break;
-		}
-
-		CCS->soundh->stopSound(soundChannel);
-	}
-
-	//Update cursor so icon can change if needed when it reappears; doesn;'t apply if a dialog box pops up at the end of the movement
-	if (!showingDialog->get())
-		GH.fakeMouseMove();
-
-	CGI->mh->waitForOngoingAnimations();
-	setMovementStatus(false);
-}
-
 void CPlayerInterface::showWorldViewEx(const std::vector<ObjectPosInfo>& objectPositions, bool showTerrain)
 {
 	EVENT_HANDLER_CALLED_BY_CLIENT;

client/CPlayerInterface.h

@@ -47,6 +47,7 @@ class KeyInterested;
 class MotionInterested;
 class PlayerLocalState;
 class TimeInterested;
+class HeroMovementController;
 
 namespace boost
 {
@@ -57,7 +58,6 @@ namespace boost
 /// Central class for managing user interface logic
 class CPlayerInterface : public CGameInterface, public IUpdateable
 {
-	bool duringMovement;
 	bool ignoreEvents;
 	size_t numOfMovedArts;
 
@@ -67,9 +67,7 @@ class CPlayerInterface : public CGameInterface, public IUpdateable
 
 	std::list<std::shared_ptr<CInfoWindow>> dialogs; //queue of dialogs awaiting to be shown (not currently shown!)
 
-	ObjectInstanceID destinationTeleport; //contain -1 or object id if teleportation
+	std::unique_ptr<HeroMovementController> movementController;
-	int3 destinationTeleportPos;
-
 public: // TODO: make private
 	std::shared_ptr<Environment> env;
 
@@ -122,7 +120,7 @@ protected: // Call-ins from server, should not be called directly, but only via
 	void showInfoDialog(EInfoWindowMode type, const std::string & text, const std::vector<Component> & components, int soundID) override;
 	void showRecruitmentDialog(const CGDwelling *dwelling, const CArmedInstance *dst, int level) override;
 	void showBlockingDialog(const std::string &text, const std::vector<Component> &components, QueryID askID, const int soundID, bool selection, bool cancel) override; //Show a dialog, player must take decision. If selection then he has to choose between one of given components, if cancel he is allowed to not choose. After making choice, CCallback::selectionMade should be called with number of selected component (1 - n) or 0 for cancel (if allowed) and askID.
-	void showTeleportDialog(TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID) override;
+	void showTeleportDialog(const CGHeroInstance * hero, TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID) override;
 	void showGarrisonDialog(const CArmedInstance *up, const CGHeroInstance *down, bool removableUnits, QueryID queryID) override;
 	void showMapObjectSelectDialog(QueryID askID, const Component & icon, const MetaString & title, const MetaString & description, const std::vector<ObjectInstanceID> & objects) override;
 	void showMarketWindow(const IMarket *market, const CGHeroInstance *visitor) override;
@@ -196,7 +194,6 @@ public: // public interface for use by client via LOCPLINT access
 	void showInfoDialogAndWait(std::vector<Component> & components, const MetaString & text);
 	void showYesNoDialog(const std::string &text, CFunctionList<void()> onYes, CFunctionList<void()> onNo, const std::vector<std::shared_ptr<CComponent>> & components = std::vector<std::shared_ptr<CComponent>>());
 
-	void stopMovement();
 	void moveHero(const CGHeroInstance *h, const CGPath& path);
 
 	void tryDigging(const CGHeroInstance *h);
@@ -222,7 +219,6 @@ private:
 		{
 			owner.ignoreEvents = false;
 		};
-
 	};
 
 	void heroKilled(const CGHeroInstance* hero);
@@ -231,9 +227,6 @@ private:
 	void acceptTurn(QueryID queryID); //used during hot seat after your turn message is close
 	void initializeHeroTownList();
 	int getLastIndex(std::string namePrefix);
-	void doMoveHero(const CGHeroInstance *h, CGPath path);
-	void setMovementStatus(bool value);
-
 };
 
 /// Provides global access to instance of interface of currently active player

client/HeroMovementController.cpp

@@ -0,0 +1,380 @@
+/*
+ * HeroMovementController.cpp, part of VCMI engine
+ *
+ * Authors: listed in file AUTHORS in main folder
+ *
+ * License: GNU General Public License v2.0 or later
+ * Full text of license available in license.txt file, in main folder
+ *
+ */
+#include "StdInc.h"
+#include "HeroMovementController.h"
+
+#include "CGameInfo.h"
+#include "CMusicHandler.h"
+#include "CPlayerInterface.h"
+#include "PlayerLocalState.h"
+#include "adventureMap/AdventureMapInterface.h"
+#include "eventsSDL/InputHandler.h"
+#include "gui/CGuiHandler.h"
+#include "gui/CursorHandler.h"
+#include "mapView/mapHandler.h"
+
+#include "../CCallback.h"
+
+#include "../lib/pathfinder/CGPathNode.h"
+#include "../lib/mapObjects/CGHeroInstance.h"
+#include "../lib/RoadHandler.h"
+#include "../lib/TerrainHandler.h"
+#include "../lib/NetPacks.h"
+
+bool HeroMovementController::isHeroMovingThroughGarrison(const CGHeroInstance * hero, const CArmedInstance * garrison) const
+{
+	if(!duringMovement)
+		return false;
+
+	if(!LOCPLINT->localState->hasPath(hero))
+		return false;
+
+	if(garrison->visitableAt(LOCPLINT->localState->getPath(hero).lastNode().coord))
+		return false; // hero want to enter garrison, not pass through it
+
+	return true;
+}
+
+bool HeroMovementController::isHeroMoving() const
+{
+	return duringMovement;
+}
+
+void HeroMovementController::onPlayerTurnStarted()
+{
+	assert(duringMovement == false);
+	assert(stoppingMovement == false);
+	duringMovement = false;
+	currentlyMovingHero = nullptr;
+}
+
+void HeroMovementController::onBattleStarted()
+{
+	// when battle starts, game will send battleStart pack *before* movement confirmation
+	// and since network thread wait for battle intro to play, movement confirmation will only happen after intro
+	// leading to several bugs, such as blocked input during intro
+	requestMovementAbort();
+}
+
+void HeroMovementController::showTeleportDialog(const CGHeroInstance * hero, TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID)
+{
+	if (impassable || exits.empty()) //FIXME: why we even have this dialog in such case?
+	{
+		LOCPLINT->cb->selectionMade(-1, askID);
+		return;
+	}
+
+	// Player entered teleporter
+	// Check whether hero that has entered teleporter has paths that goes through teleporter and select appropriate exit
+	// othervice, ask server to select one randomly by sending invalid (-1) value as answer
+	assert(waitingForQueryApplyReply == false);
+	waitingForQueryApplyReply = true;
+
+	if(!LOCPLINT->localState->hasPath(hero))
+	{
+		// Hero enters teleporter without specifying exit - select it randomly
+		LOCPLINT->cb->selectionMade(-1, askID);
+		return;
+	}
+
+	const auto & heroPath = LOCPLINT->localState->getPath(hero);
+	const auto & nextNode = heroPath.nextNode();
+
+	for(size_t i = 0; i < exits.size(); ++i)
+	{
+		const auto * teleporter = LOCPLINT->cb->getObj(exits[i].first);
+
+		if(teleporter && teleporter->visitableAt(nextNode.coord))
+		{
+			// Remove this node from path - it will be covered by teleportation
+			//LOCPLINT->localState->removeLastNode(hero);
+			LOCPLINT->cb->selectionMade(i, askID);
+			return;
+		}
+	}
+
+	assert(0); // exit not found? How?
+	LOCPLINT->cb->selectionMade(-1, askID);
+	return;
+}
+
+void HeroMovementController::updatePath(const CGHeroInstance * hero, const TryMoveHero & details)
+{
+	// Once hero moved (or attempted to move) we need to update path
+	// to make sure that it is still valid or remove it completely if destination has been reached
+
+	if(hero->tempOwner != LOCPLINT->playerID)
+		return;
+
+	if(!LOCPLINT->localState->hasPath(hero))
+		return; // may happen when hero teleports
+
+	assert(LOCPLINT->makingTurn);
+
+	bool directlyAttackingCreature = details.attackedFrom.has_value() && LOCPLINT->localState->getPath(hero).lastNode().coord == details.attackedFrom;
+
+	int3 desiredTarget = LOCPLINT->localState->getPath(hero).nextNode().coord;
+	int3 actualTarget = hero->convertToVisitablePos(details.end);
+
+	//don't erase path when revisiting with spacebar
+	bool heroChangedTile = details.start != details.end;
+
+	if(heroChangedTile)
+	{
+		if(desiredTarget != actualTarget)
+		{
+			//invalidate path - movement was not along current path
+			//possible reasons: teleport, visit of object with "blocking visit" property
+			LOCPLINT->localState->erasePath(hero);
+		}
+		else
+		{
+			//movement along desired path - remove one node and keep rest of path
+			LOCPLINT->localState->removeLastNode(hero);
+		}
+
+		if(directlyAttackingCreature)
+			LOCPLINT->localState->erasePath(hero);
+	}
+}
+
+void HeroMovementController::onTryMoveHero(const CGHeroInstance * hero, const TryMoveHero & details)
+{
+	// Server initiated movement -> start movement animation
+	// Note that this movement is not necessarily of owned heroes - other players movement will also pass through this method
+
+	if(details.result == TryMoveHero::EMBARK || details.result == TryMoveHero::DISEMBARK)
+	{
+		if(hero->getRemovalSound() && hero->tempOwner == LOCPLINT->playerID)
+			CCS->soundh->playSound(hero->getRemovalSound().value());
+	}
+
+	bool directlyAttackingCreature =
+		details.attackedFrom.has_value() &&
+		LOCPLINT->localState->hasPath(hero) &&
+		LOCPLINT->localState->getPath(hero).lastNode().coord == details.attackedFrom;
+
+	std::unordered_set<int3> changedTiles {
+		hero->convertToVisitablePos(details.start),
+		hero->convertToVisitablePos(details.end)
+	};
+	adventureInt->onMapTilesChanged(changedTiles);
+	adventureInt->onHeroMovementStarted(hero);
+
+	updatePath(hero, details);
+
+	if(details.stopMovement())
+	{
+		if(duringMovement)
+			endMove(hero);
+		return;
+	}
+
+	// We are in network thread
+	// Block netpack processing until movement animation is over
+	CGI->mh->waitForOngoingAnimations();
+
+	//move finished
+	adventureInt->onHeroChanged(hero);
+
+	// Hero attacked creature, set direction to face it.
+	if(directlyAttackingCreature)
+	{
+		// Get direction to attacker.
+		int3 posOffset = *details.attackedFrom - details.end + int3(2, 1, 0);
+		static const ui8 dirLookup[3][3] =
+		{
+			{ 1, 2, 3 },
+			{ 8, 0, 4 },
+			{ 7, 6, 5 }
+		};
+
+		//FIXME: better handling of this case without const_cast
+		const_cast<CGHeroInstance *>(hero)->moveDir = dirLookup[posOffset.y][posOffset.x];
+	}
+}
+
+void HeroMovementController::onQueryReplyApplied()
+{
+	if(duringMovement)
+	{
+		// Server accepted our TeleportDialog query reply and moved hero
+		// Continue moving alongside our path, if any
+
+		assert(waitingForQueryApplyReply);
+		waitingForQueryApplyReply = false;
+		onMoveHeroApplied();
+	}
+}
+
+void HeroMovementController::onMoveHeroApplied()
+{
+	// at this point, server have finished processing of hero movement request
+	// as well as all side effectes from movement, such as object visit or combat start
+
+	// this was request to move alongside path from player, but either another player or teleport action
+	if(!duringMovement)
+		return;
+
+	// hero has moved onto teleporter and activated it
+	// in this case next movement should be done only after query reply has been acknowledged
+	// and hero has been moved to teleport destination
+	if(waitingForQueryApplyReply)
+		return;
+
+	if(GH.input().ignoreEventsUntilInput())
+		stoppingMovement = true;
+
+	assert(currentlyMovingHero);
+	const auto * hero = currentlyMovingHero;
+
+	bool canMove = LOCPLINT->localState->hasPath(hero) && LOCPLINT->localState->getPath(hero).nextNode().turns == 0;
+	bool wantStop = stoppingMovement;
+	bool canStop = !canMove || canHeroStopAtNode(LOCPLINT->localState->getPath(hero).currNode());
+
+	if(!canMove || (wantStop && canStop))
+	{
+		endMove(hero);
+	}
+	else
+	{
+		moveOnce(hero, LOCPLINT->localState->getPath(hero));
+	}
+}
+
+void HeroMovementController::requestMovementAbort()
+{
+	if(duringMovement)
+		endMove(currentlyMovingHero);
+}
+
+void HeroMovementController::endMove(const CGHeroInstance * hero)
+{
+	assert(duringMovement == true);
+	assert(currentlyMovingHero != nullptr);
+	duringMovement = false;
+	stoppingMovement = false;
+	currentlyMovingHero = nullptr;
+	stopMovementSound();
+	adventureInt->onHeroChanged(hero);
+	CCS->curh->show();
+}
+
+AudioPath HeroMovementController::getMovementSoundFor(const CGHeroInstance * hero, int3 posPrev, int3 posNext, EPathNodeAction moveType)
+{
+	if(moveType == EPathNodeAction::TELEPORT_BATTLE || moveType == EPathNodeAction::TELEPORT_BLOCKING_VISIT || moveType == EPathNodeAction::TELEPORT_NORMAL)
+		return {};
+
+	if(moveType == EPathNodeAction::EMBARK || moveType == EPathNodeAction::DISEMBARK)
+		return {};
+
+	if(moveType == EPathNodeAction::BLOCKING_VISIT)
+		return {};
+
+	// flying movement sound
+	if(hero->hasBonusOfType(BonusType::FLYING_MOVEMENT))
+		return AudioPath::builtin("HORSE10.wav");
+
+	auto prevTile = LOCPLINT->cb->getTile(posPrev);
+	auto nextTile = LOCPLINT->cb->getTile(posNext);
+
+	auto prevRoad = prevTile->roadType;
+	auto nextRoad = nextTile->roadType;
+	bool movingOnRoad = prevRoad->getId() != Road::NO_ROAD && nextRoad->getId() != Road::NO_ROAD;
+
+	if(movingOnRoad)
+		return nextTile->terType->horseSound;
+	else
+		return nextTile->terType->horseSoundPenalty;
+};
+
+void HeroMovementController::updateMovementSound(const CGHeroInstance * h, int3 posPrev, int3 nextCoord, EPathNodeAction moveType)
+{
+	// Start a new sound for the hero movement or let the existing one carry on.
+	AudioPath newSoundName = getMovementSoundFor(h, posPrev, nextCoord, moveType);
+
+	if(newSoundName != currentMovementSoundName)
+	{
+		currentMovementSoundName = newSoundName;
+
+		if(currentMovementSoundChannel != -1)
+			CCS->soundh->stopSound(currentMovementSoundChannel);
+
+		if(!currentMovementSoundName.empty())
+			currentMovementSoundChannel = CCS->soundh->playSound(currentMovementSoundName, -1, true);
+		else
+			currentMovementSoundChannel = -1;
+	}
+}
+
+void HeroMovementController::stopMovementSound()
+{
+	if(currentMovementSoundChannel != -1)
+		CCS->soundh->stopSound(currentMovementSoundChannel);
+	currentMovementSoundChannel = -1;
+	currentMovementSoundName = AudioPath();
+}
+
+bool HeroMovementController::canHeroStopAtNode(const CGPathNode & node) const
+{
+	if(node.layer != EPathfindingLayer::LAND && node.layer != EPathfindingLayer::SAIL)
+		return false;
+
+	if(node.accessible != EPathAccessibility::ACCESSIBLE)
+		return false;
+
+	return true;
+}
+
+void HeroMovementController::requestMovementStart(const CGHeroInstance * h, const CGPath & path)
+{
+	assert(duringMovement == false);
+	duringMovement = true;
+	currentlyMovingHero = h;
+
+	CCS->curh->hide();
+	moveOnce(h, path);
+}
+
+void HeroMovementController::moveOnce(const CGHeroInstance * h, const CGPath & path)
+{
+	// Moves hero once, sends request to server and immediately returns
+	// movement alongside paths will be done on receiving response from server
+
+	assert(duringMovement == true);
+
+	const auto & currNode = path.currNode();
+	const auto & nextNode = path.nextNode();
+
+	assert(nextNode.turns == 0);
+	assert(currNode.coord == h->visitablePos());
+
+	int3 nextCoord = h->convertFromVisitablePos(nextNode.coord);
+
+	if(nextNode.isTeleportAction())
+	{
+		stopMovementSound();
+		logGlobal->trace("Requesting hero teleportation to %s", nextNode.coord.toString());
+		LOCPLINT->cb->moveHero(h, nextCoord, false);
+		return;
+	}
+	else
+	{
+		updateMovementSound(h, currNode.coord, nextNode.coord, nextNode.action);
+
+		assert(h->pos.z == nextNode.coord.z); // Z should change only if it's movement via teleporter and in this case this code shouldn't be executed at all
+
+		logGlobal->trace("Requesting hero movement to %s", nextNode.coord.toString());
+
+		bool useTransit = nextNode.layer == EPathfindingLayer::AIR || nextNode.layer == EPathfindingLayer::WATER;
+		LOCPLINT->cb->moveHero(h, nextCoord, useTransit);
+		return;
+	}
+}

client/HeroMovementController.h

@@ -0,0 +1,74 @@
+/*
+ * HeroMovementController.h, part of VCMI engine
+ *
+ * Authors: listed in file AUTHORS in main folder
+ *
+ * License: GNU General Public License v2.0 or later
+ * Full text of license available in license.txt file, in main folder
+ *
+ */
+#pragma once
+
+#include "../lib/constants/EntityIdentifiers.h"
+#include "../lib/int3.h"
+#include "../lib/filesystem/ResourcePath.h"
+
+VCMI_LIB_NAMESPACE_BEGIN
+using TTeleportExitsList = std::vector<std::pair<ObjectInstanceID, int3>>;
+
+class CGHeroInstance;
+class CArmedInstance;
+struct CGPathNode;
+
+struct CGPath;
+struct TryMoveHero;
+enum class EPathNodeAction : ui8;
+VCMI_LIB_NAMESPACE_END
+
+class HeroMovementController
+{
+	/// there is an ongoing movement loop, in one or another stage
+	bool duringMovement = false;
+	/// movement was requested to be terminated, e.g. by player or due to inability to move
+	bool stoppingMovement = false;
+
+	bool waitingForQueryApplyReply = false;
+
+	const CGHeroInstance * currentlyMovingHero = nullptr;
+	AudioPath currentMovementSoundName;
+	int currentMovementSoundChannel = -1;
+
+	bool canHeroStopAtNode(const CGPathNode & node) const;
+
+	void updatePath(const CGHeroInstance * hero, const TryMoveHero & details);
+
+	/// Moves hero 1 tile / path node
+	void moveOnce(const CGHeroInstance * h, const CGPath & path);
+
+	void endMove(const CGHeroInstance * h);
+
+	AudioPath getMovementSoundFor(const CGHeroInstance * hero, int3 posPrev, int3 posNext, EPathNodeAction moveType);
+	void updateMovementSound(const CGHeroInstance * hero, int3 posPrev, int3 posNext, EPathNodeAction moveType);
+	void stopMovementSound();
+
+public:
+	// const queries
+
+	/// Returns true if hero should move through garrison without displaying garrison dialog
+	bool isHeroMovingThroughGarrison(const CGHeroInstance * hero, const CArmedInstance * garrison) const;
+
+	/// Returns true if there is an ongoing hero movement process
+	bool isHeroMoving() const;
+
+	// netpack handlers
+	void onMoveHeroApplied();
+	void onQueryReplyApplied();
+	void onPlayerTurnStarted();
+	void onBattleStarted();
+	void showTeleportDialog(const CGHeroInstance * hero, TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID);
+	void onTryMoveHero(const CGHeroInstance * hero, const TryMoveHero & details);
+
+	// UI handlers
+	void requestMovementStart(const CGHeroInstance * h, const CGPath & path);
+	void requestMovementAbort();
+};

client/NetPacksClient.cpp

@@ -703,7 +703,8 @@ void ApplyClientNetPackVisitor::visitExchangeDialog(ExchangeDialog & pack)
 
 void ApplyClientNetPackVisitor::visitTeleportDialog(TeleportDialog & pack)
 {
-	callOnlyThatInterface(cl, pack.player, &CGameInterface::showTeleportDialog, pack.channel, pack.exits, pack.impassable, pack.queryID);
+	const CGHeroInstance *h = cl.getHero(pack.hero);
+	callOnlyThatInterface(cl, h->getOwner(), &CGameInterface::showTeleportDialog, h, pack.channel, pack.exits, pack.impassable, pack.queryID);
 }
 
 void ApplyClientNetPackVisitor::visitMapObjectSelectDialog(MapObjectSelectDialog & pack)

client/adventureMap/AdventureMapInterface.cpp

@@ -531,7 +531,8 @@ void AdventureMapInterface::onTileLeftClicked(const int3 &mapPos)
 			if(LOCPLINT->localState->hasPath(currentHero) &&
 			   LOCPLINT->localState->getPath(currentHero).endPos() == mapPos)//we'll be moving
 			{
-				if(!CGI->mh->hasOngoingAnimations())
+				assert(!CGI->mh->hasOngoingAnimations());
+				if(!CGI->mh->hasOngoingAnimations() && LOCPLINT->localState->getPath(currentHero).nextNode().turns == 0)
 					LOCPLINT->moveHero(currentHero, LOCPLINT->localState->getPath(currentHero));
 				return;
 			}

lib/CGameInterface.h

@@ -103,7 +103,7 @@ public:
 
 	// all stacks operations between these objects become allowed, interface has to call onEnd when done
 	virtual void showGarrisonDialog(const CArmedInstance *up, const CGHeroInstance *down, bool removableUnits, QueryID queryID) = 0;
-	virtual void showTeleportDialog(TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID) = 0;
+	virtual void showTeleportDialog(const CGHeroInstance * hero, TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID) = 0;
 	virtual void showMapObjectSelectDialog(QueryID askID, const Component & icon, const MetaString & title, const MetaString & description, const std::vector<ObjectInstanceID> & objects) = 0;
 	virtual void finish(){}; //if for some reason we want to end
 

lib/NetPacks.h

@@ -1436,12 +1436,12 @@ struct DLL_LINKAGE TeleportDialog : public Query
 {
 	TeleportDialog() = default;
 
-	TeleportDialog(const PlayerColor & Player, const TeleportChannelID & Channel)
+	TeleportDialog(const ObjectInstanceID & hero, const TeleportChannelID & Channel)
-		: player(Player)
+		: hero(hero)
 		, channel(Channel)
 	{
 	}
-	PlayerColor player;
+	ObjectInstanceID hero;
 	TeleportChannelID channel;
 	TTeleportExitsList exits;
 	bool impassable = false;
@@ -1451,7 +1451,7 @@ struct DLL_LINKAGE TeleportDialog : public Query
 	template <typename Handler> void serialize(Handler & h, const int version)
 	{
 		h & queryID;
-		h & player;
+		h & hero;
 		h & channel;
 		h & exits;
 		h & impassable;

lib/mapObjects/CGObjectInstance.cpp

@@ -64,15 +64,18 @@ void CGObjectInstance::setOwner(const PlayerColor & ow)
 {
 	tempOwner = ow;
 }
-int CGObjectInstance::getWidth() const//returns width of object graphic in tiles
+
+int CGObjectInstance::getWidth() const
 {
 	return appearance->getWidth();
 }
-int CGObjectInstance::getHeight() const //returns height of object graphic in tiles
+
+int CGObjectInstance::getHeight() const
 {
 	return appearance->getHeight();
 }
-bool CGObjectInstance::visitableAt(int x, int y) const //returns true if object is visitable at location (x, y) form left top tile of image (x, y in tiles)
+
+bool CGObjectInstance::visitableAt(int x, int y) const
 {
 	return appearance->isVisitableAt(pos.x - x, pos.y - y);
 }
@@ -86,6 +89,20 @@ bool CGObjectInstance::coveringAt(int x, int y) const
 	return appearance->isVisibleAt(pos.x - x, pos.y - y);
 }
 
+bool CGObjectInstance::visitableAt(const int3 & testPos) const
+{
+	return pos.z == testPos.z && appearance->isVisitableAt(pos.x - testPos.x, pos.y - testPos.y);
+}
+bool CGObjectInstance::blockingAt(const int3 & testPos) const
+{
+	return pos.z == testPos.z && appearance->isBlockedAt(pos.x - testPos.x, pos.y - testPos.y);
+}
+
+bool CGObjectInstance::coveringAt(const int3 & testPos) const
+{
+	return pos.z == testPos.z && appearance->isVisibleAt(pos.x - testPos.x, pos.y - testPos.y);
+}
+
 std::set<int3> CGObjectInstance::getBlockedPos() const
 {
 	std::set<int3> ret;

lib/mapObjects/CGObjectInstance.h

@@ -60,12 +60,17 @@ public:
 
 	int getWidth() const; //returns width of object graphic in tiles
 	int getHeight() const; //returns height of object graphic in tiles
-	bool visitableAt(int x, int y) const; //returns true if object is visitable at location (x, y) (h3m pos)
 	int3 visitablePos() const override;
 	int3 getPosition() const override;
 	int3 getTopVisiblePos() const;
+	bool visitableAt(int x, int y) const; //returns true if object is visitable at location (x, y) (h3m pos)
 	bool blockingAt(int x, int y) const; //returns true if object is blocking location (x, y) (h3m pos)
 	bool coveringAt(int x, int y) const; //returns true if object covers with picture location (x, y) (h3m pos)
+
+	bool visitableAt(const int3 & pos) const; //returns true if object is visitable at location (x, y) (h3m pos)
+	bool blockingAt (const int3 & pos) const; //returns true if object is blocking location (x, y) (h3m pos)
+	bool coveringAt (const int3 & pos) const; //returns true if object covers with picture location (x, y) (h3m pos)
+
 	std::set<int3> getBlockedPos() const; //returns set of positions blocked by this object
 	std::set<int3> getBlockedOffsets() const; //returns set of relative positions blocked by this object
 

lib/mapObjects/MiscObjects.cpp

@@ -453,7 +453,7 @@ TeleportChannelID CGMonolith::findMeChannel(const std::vector<Obj> & IDs, int Su
 
 void CGMonolith::onHeroVisit( const CGHeroInstance * h ) const
 {
-	TeleportDialog td(h->tempOwner, channel);
+	TeleportDialog td(h->id, channel);
 	if(isEntrance())
 	{
 		if(cb->isTeleportChannelBidirectional(channel) && 1 < cb->getTeleportChannelExits(channel).size())
@@ -527,7 +527,7 @@ void CGMonolith::initObj(CRandomGenerator & rand)
 
 void CGSubterraneanGate::onHeroVisit( const CGHeroInstance * h ) const
 {
-	TeleportDialog td(h->tempOwner, channel);
+	TeleportDialog td(h->id, channel);
 	if(cb->isTeleportChannelImpassable(channel))
 	{
 		h->showInfoDialog(153);//Just inside the entrance you find a large pile of rubble blocking the tunnel. You leave discouraged.
@@ -611,7 +611,7 @@ void CGSubterraneanGate::postInit() //matches subterranean gates into pairs
 
 void CGWhirlpool::onHeroVisit( const CGHeroInstance * h ) const
 {
-	TeleportDialog td(h->tempOwner, channel);
+	TeleportDialog td(h->id, channel);
 	if(cb->isTeleportChannelImpassable(channel))
 	{
 		logGlobal->debug("Cannot find exit whirlpool for %d at %s", id.getNum(), pos.toString());

lib/pathfinder/CGPathNode.cpp

@@ -24,6 +24,24 @@ static bool canSeeObj(const CGObjectInstance * obj)
 	return obj != nullptr && obj->ID != Obj::EVENT;
 }
 
+const CGPathNode & CGPath::currNode() const
+{
+	assert(nodes.size() > 1);
+	return nodes[nodes.size()-1];
+}
+
+const CGPathNode & CGPath::nextNode() const
+{
+	assert(nodes.size() > 1);
+	return nodes[nodes.size()-2];
+}
+
+const CGPathNode & CGPath::lastNode() const
+{
+	assert(nodes.size() > 1);
+	return nodes[0];
+}
+
 int3 CGPath::startPos() const
 {
 	return nodes[nodes.size()-1].coord;

lib/pathfinder/CGPathNode.h

@@ -143,6 +143,18 @@ struct DLL_LINKAGE CGPathNode
 		return turns < 255;
 	}
 
+	bool isTeleportAction() const
+	{
+		if (action != EPathNodeAction::TELEPORT_NORMAL &&
+			action != EPathNodeAction::TELEPORT_BLOCKING_VISIT &&
+			action != EPathNodeAction::TELEPORT_BATTLE)
+		{
+			return false;
+		}
+
+		return true;
+	}
+
 	using TFibHeap = boost::heap::fibonacci_heap<CGPathNode *, boost::heap::compare<NodeComparer<CGPathNode>>>;
 
 	TFibHeap::handle_type pqHandle;
@@ -156,6 +168,13 @@ struct DLL_LINKAGE CGPath
 {
 	std::vector<CGPathNode> nodes; //just get node by node
 
+	/// Starting position of path, matches location of hero
+	const CGPathNode & currNode() const;
+	/// First node in path, this is where hero will move next
+	const CGPathNode & nextNode() const;
+	/// Last node in path, this is what hero wants to reach in the end
+	const CGPathNode & lastNode() const;
+
 	int3 startPos() const; // start point
 	int3 endPos() const; //destination point
 };

server/CGameHandler.cpp

@@ -1111,29 +1111,36 @@ bool CGameHandler::moveHero(ObjectInstanceID hid, int3 dst, ui8 teleporting, boo
 	const bool standAtObstacle = t.blocked && !t.visitable;
 	const bool standAtWater = !h->boat && t.terType->isWater() && (t.visitableObjects.empty() || !t.visitableObjects.back()->isCoastVisitable());
 	
-	//it's a rock or blocked and not visitable tile
+	auto const complainRet = [&](const std::string & message){
-	//OR hero is on land and dest is water and (there is not present only one object - boat)
-	if (((!t.terType->isPassable() || (standAtObstacle && !canFly))
-			&& complain("Cannot move hero, destination tile is blocked!"))
-		|| ((standAtWater && !canFly && !canWalkOnSea)  //hero is not on boat/water walking and dst water tile doesn't contain boat/hero (objs visitable from land) -> we test back cause boat may be on top of another object (#276)
-			&& complain("Cannot move hero, destination tile is on water!"))
-		|| ((h->boat && h->boat->layer == EPathfindingLayer::SAIL && t.terType->isLand() && t.blocked)
-			&& complain("Cannot disembark hero, tile is blocked!"))
-		|| ((distance(h->pos, dst) >= 1.5 && !teleporting)
-			&& complain("Tiles are not neighboring!"))
-		|| ((h->inTownGarrison)
-			&& complain("Can not move garrisoned hero!"))
-		|| (((int)h->movementPointsRemaining() < cost  &&  dst != h->pos  &&  !teleporting)
-			&& complain("Hero doesn't have any movement points left!"))
-		|| ((transit && !canFly && !CGTeleport::isTeleport(t.topVisitableObj()))
-			&& complain("Hero cannot transit over this tile!"))
-		/*|| (states.checkFlag(h->tempOwner, &PlayerStatus::engagedIntoBattle)
-			&& complain("Cannot move hero during the battle"))*/)
-	{
 		//send info about movement failure
+		complain(message);
 		sendAndApply(&tmh);
 		return false;
-	}
+	};
+
+	//it's a rock or blocked and not visitable tile
+	//OR hero is on land and dest is water and (there is not present only one object - boat)
+	if (!t.terType->isPassable() || (standAtObstacle && !canFly))
+		complainRet("Cannot move hero, destination tile is blocked!");
+
+	//hero is not on boat/water walking and dst water tile doesn't contain boat/hero (objs visitable from land) -> we test back cause boat may be on top of another object (#276)
+	if(standAtWater && !canFly && !canWalkOnSea)
+		complainRet("Cannot move hero, destination tile is on water!");
+
+	if(h->boat && h->boat->layer == EPathfindingLayer::SAIL && t.terType->isLand() && t.blocked)
+		complainRet("Cannot disembark hero, tile is blocked!");
+
+	if(distance(h->pos, dst) >= 1.5 && !teleporting)
+		complainRet("Tiles are not neighboring!");
+
+	if(h->inTownGarrison)
+		complainRet("Can not move garrisoned hero!");
+
+	if(h->movementPointsRemaining() < cost && dst != h->pos && !teleporting)
+		complainRet("Hero doesn't have any movement points left!");
+
+	if (transit && !canFly && !(canWalkOnSea && t.terType->isWater()))
+		complainRet("Hero cannot transit over this tile!");
 
 	//several generic blocks of code
 
@@ -1252,7 +1259,7 @@ bool CGameHandler::moveHero(ObjectInstanceID hid, int3 dst, ui8 teleporting, boo
 			if (CGTeleport::isTeleport(t.topVisitableObj()))
 				visitDest = DONT_VISIT_DEST;
 
-			if (canFly)
+			if (canFly || (canWalkOnSea && t.terType->isWater()))
 			{
 				lookForGuards = IGNORE_GUARDS;
 				visitDest = DONT_VISIT_DEST;

server/queries/MapQueries.cpp

@@ -171,7 +171,7 @@ CTeleportDialogQuery::CTeleportDialogQuery(CGameHandler * owner, const TeleportD
 	CDialogQuery(owner)
 {
 	this->td = td;
-	addPlayer(td.player);
+	addPlayer(gh->getHero(td.hero)->getOwner());
 }
 
 CHeroLevelUpDialogQuery::CHeroLevelUpDialogQuery(CGameHandler * owner, const HeroLevelUp & Hlu, const CGHeroInstance * Hero):