Minor cleanup of hero movemen code

client/CPlayerInterface.cpp

@@ -980,7 +980,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->movementStopRequested(); // interrupt movement to show dialog
 		GH.windows().pushWindow(temp);
 	}
 	else
@@ -1003,7 +1003,7 @@ void CPlayerInterface::showYesNoDialog(const std::string &text, CFunctionList<vo
 {
 	boost::unique_lock<boost::recursive_mutex> un(*pim);
 
-	stopMovement();
+	movementController->movementStopRequested();
 	LOCPLINT->showingDialog->setn(true);
 	CInfoWindow::showYesNoDialog(text, components, onYes, onNo, playerID);
 }
@@ -1013,7 +1013,7 @@ void CPlayerInterface::showBlockingDialog( const std::string &text, const std::v
 	EVENT_HANDLER_CALLED_BY_CLIENT;
 	waitWhileDialog();
 
-	stopMovement();
+	movementController->movementStopRequested();
 	CCS->soundh->playSound(static_cast<soundBase::soundID>(soundID));
 
 	if (!selection && cancel) //simple yes/no dialog
@@ -1160,6 +1160,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;
@@ -1173,7 +1178,7 @@ void CPlayerInterface::moveHero( const CGHeroInstance *h, const CGPath& path )
 	if (localState->isHeroSleeping(h))
 		localState->setHeroAwaken(h);
 
-	movementController->doMoveHero(h, path);
+	movementController->movementStartRequested(h, path);
 }
 
 void CPlayerInterface::showGarrisonDialog( const CArmedInstance *up, const CGHeroInstance *down, bool removableUnits, QueryID queryID)
@@ -1181,7 +1186,7 @@ void CPlayerInterface::showGarrisonDialog( const CArmedInstance *up, const CGHer
 	EVENT_HANDLER_CALLED_BY_CLIENT;
 	auto onEnd = [=](){ cb->selectionMade(0, queryID); };
 
-	if (movementController->isHeroMovingThroughGarrison(down))
+	if (movementController->isHeroMovingThroughGarrison(down, up))
 	{
 		onEnd();
 		return;
@@ -1617,13 +1622,6 @@ void CPlayerInterface::battleNewRoundFirst(const BattleID & battleID)
 	battleInt->newRoundFirst();
 }
 
-void CPlayerInterface::stopMovement()
-{
-	movementController->movementStopRequested();
-
-
-}
-
 void CPlayerInterface::showMarketWindow(const IMarket *market, const CGHeroInstance *visitor)
 {
 	EVENT_HANDLER_CALLED_BY_CLIENT;

client/CPlayerInterface.h

@@ -194,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);

client/HeroMovementController.cpp

@@ -30,6 +30,20 @@
 #include "../lib/NetPacks.h"
 #include "../lib/CondSh.h"
 
+// Code flow of movement process:
+// - Player requests movement (e.g. press "move" button)
+// -> calls doMoveHero (GUI thread)
+// -> -> sends LOCPLINT->cb->moveHero
+// - Server sends reply
+// -> calls heroMoved (NETWORK thread)
+// -> -> animation starts, sound starts
+// -> -> waits for animation to finish
+// -> -> if player have requested stop and path is not finished, calls doMoveHero again
+//
+// - Players requests movement stop (e.g. mouse click during movement)
+// -> calls movementStopRequested
+// -> -> sets flag that movement should be aborted
+
 HeroMovementController::HeroMovementController()
 {
 	destinationTeleport = ObjectInstanceID();
@@ -41,19 +55,23 @@ void HeroMovementController::setMovementStatus(bool value)
 {
 	duringMovement = value;
 	if (value)
-	{
 		CCS->curh->hide();
-	}
 	else
-	{
 		CCS->curh->show();
-	}
 }
 
-bool HeroMovementController::isHeroMovingThroughGarrison(const CGHeroInstance * hero) const
+bool HeroMovementController::isHeroMovingThroughGarrison(const CGHeroInstance * hero, const CArmedInstance * garrison) const
 {
-	//to ignore calls on passing through garrisons
-	return (movementState == EMoveState::DURING_MOVE && LOCPLINT->localState->hasPath(hero) && LOCPLINT->localState->getPath(hero).nodes.size() > 1);
+	assert(LOCPLINT->localState->hasPath(hero));
+	assert(movementState == EMoveState::DURING_MOVE);
+
+	if (!LOCPLINT->localState->hasPath(hero))
+		return false;
+
+	if (garrison->visitableAt(*LOCPLINT->localState->getLastTile(hero)))
+		return false; // hero want to enter garrison, not pass through it
+
+	return true;
 }
 
 bool HeroMovementController::isHeroMoving() const
@@ -94,6 +112,7 @@ void HeroMovementController::onPlayerTurnStarted()
 
 void HeroMovementController::onBattleStarted()
 {
+	assert(movementState == EMoveState::STOP_MOVE);
 	// 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
@@ -102,11 +121,12 @@ void HeroMovementController::onBattleStarted()
 
 void HeroMovementController::showTeleportDialog(TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID)
 {
-	int choosenExit = -1;
+	assert(destinationTeleport != ObjectInstanceID::NONE);
+
 	auto neededExit = std::make_pair(destinationTeleport, destinationTeleportPos);
-	if (destinationTeleport != ObjectInstanceID() && vstd::contains(exits, neededExit))
-		choosenExit = vstd::find_pos(exits, neededExit);
+	assert(vstd::contains(exits, neededExit));
 
+	int choosenExit = vstd::find_pos(exits, neededExit);
 	LOCPLINT->cb->selectionMade(choosenExit, askID);
 }
 
@@ -206,7 +226,7 @@ void HeroMovementController::movementStopRequested()
 		movementState = EMoveState::STOP_MOVE;
 }
 
-void HeroMovementController::doMoveHero(const CGHeroInstance * h, const CGPath & path)
+void HeroMovementController::movementStartRequested(const CGHeroInstance * h, const CGPath & path)
 {
 	setMovementStatus(true);
 

client/HeroMovementController.h

@@ -16,6 +16,7 @@ VCMI_LIB_NAMESPACE_BEGIN
 using TTeleportExitsList = std::vector<std::pair<ObjectInstanceID, int3>>;
 
 class CGHeroInstance;
+class CArmedInstance;
 
 struct CGPath;
 struct TryMoveHero;
@@ -38,21 +39,24 @@ class HeroMovementController
 	};
 
 	EMoveState movementState;
+
+	void setMovementStatus(bool value);
 public:
 	HeroMovementController();
 
-	bool isHeroMovingThroughGarrison(const CGHeroInstance * hero) const;
+	// const queries
+	bool isHeroMovingThroughGarrison(const CGHeroInstance * hero, const CArmedInstance * garrison) const;
 	bool isHeroMoving() const;
 
+	// netpack handlers
 	void onMoveHeroApplied();
 	void onQueryReplyApplied();
-
 	void onPlayerTurnStarted();
 	void onBattleStarted();
 	void showTeleportDialog(TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID);
 	void heroMoved(const CGHeroInstance * hero, const TryMoveHero & details);
 
+	// UI handlers
+	void movementStartRequested(const CGHeroInstance * h, const CGPath & path);
 	void movementStopRequested();
-	void doMoveHero(const CGHeroInstance * h, const CGPath & path);
-	void setMovementStatus(bool value);
 };

client/PlayerLocalState.h

@@ -76,6 +76,11 @@ public:
 	void setPath(const CGHeroInstance * h, const CGPath & path);
 	bool setPath(const CGHeroInstance * h, const int3 & destination);
 
+	/// return final node in a path, if exists
+	std::optional<int3> getLastTile(const CGHeroInstance * h) const;
+	/// return first path in a path, if exists
+	std::optional<int3> getNextTile(const CGHeroInstance * h) const;
+
 	const CGPath & getPath(const CGHeroInstance * h) const;
 	bool hasPath(const CGHeroInstance * h) const;
 

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