Fixed paths through teleport, formatting cleanup

client/CPlayerInterface.cpp

@@ -1236,6 +1236,9 @@ void CPlayerInterface::requestRealized( PackageApplied *pa )
 {
 	if(pa->packType == typeList.getTypeID<MoveHero>())
 		movementController->onMoveHeroApplied();
+
+	if(pa->packType == typeList.getTypeID<QueryReply>())
+		movementController->onQueryReplyApplied();
 }
 
 void CPlayerInterface::showHeroExchange(ObjectInstanceID hero1, ObjectInstanceID hero2)

client/HeroMovementController.cpp

@@ -1,5 +1,5 @@
 /*
- * CPlayerInterface.cpp, part of VCMI engine
+ * HeroMovementController.cpp, part of VCMI engine
  *
  * Authors: listed in file AUTHORS in main folder
  *
@@ -24,37 +24,19 @@
 
 #include "../lib/pathfinder/CGPathNode.h"
 #include "../lib/mapObjects/CGHeroInstance.h"
-#include "../lib/mapObjects/MiscObjects.h"
 #include "../lib/RoadHandler.h"
 #include "../lib/TerrainHandler.h"
 #include "../lib/NetPacks.h"
-#include "../lib/CondSh.h"
-
-std::optional<int3> HeroMovementController::getLastTile(const CGHeroInstance * hero) const
-{
-	if (!LOCPLINT->localState->hasPath(hero))
-		return std::nullopt;
-
-	return LOCPLINT->localState->getPath(hero).endPos();
-}
-
-std::optional<int3> HeroMovementController::getNextTile(const CGHeroInstance * hero) const
-{
-	if (!LOCPLINT->localState->hasPath(hero))
-		return std::nullopt;
-
-	return LOCPLINT->localState->getPath(hero).nextNode().coord;
-}
 
 bool HeroMovementController::isHeroMovingThroughGarrison(const CGHeroInstance * hero, const CArmedInstance * garrison) const
 {
 	if(!duringMovement)
 		return false;
 
-	if (!LOCPLINT->localState->hasPath(hero))
+	if(!LOCPLINT->localState->hasPath(hero))
 		return false;
 
-	if (garrison->visitableAt(*getLastTile(hero)))
+	if(garrison->visitableAt(LOCPLINT->localState->getPath(hero).lastNode().coord))
 		return false; // hero want to enter garrison, not pass through it
 
 	return true;
@@ -83,9 +65,13 @@ void HeroMovementController::onBattleStarted()
 
 void HeroMovementController::showTeleportDialog(const CGHeroInstance * hero, TeleportChannelID channel, TTeleportExitsList exits, bool impassable, QueryID askID)
 {
-	assert(hero == currentlyMovingHero);
+	// 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))
+	if(!LOCPLINT->localState->hasPath(hero))
 	{
 		// Hero enters teleporter without specifying exit - select it randomly
 		LOCPLINT->cb->selectionMade(-1, askID);
@@ -95,12 +81,14 @@ void HeroMovementController::showTeleportDialog(const CGHeroInstance * hero, Tel
 	const auto & heroPath = LOCPLINT->localState->getPath(hero);
 	const auto & nextNode = heroPath.nextNode();
 
-	for (size_t i = 0; i < exits.size(); ++i)
+	for(size_t i = 0; i < exits.size(); ++i)
 	{
 		const auto * teleporter = LOCPLINT->cb->getObj(exits[i].first);
 
-		if (teleporter && teleporter->visitableAt(nextNode.coord))
+		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;
 		}
@@ -113,27 +101,28 @@ void HeroMovementController::showTeleportDialog(const CGHeroInstance * hero, Tel
 
 void HeroMovementController::updatePath(const CGHeroInstance * hero, const TryMoveHero & details)
 {
-	if (hero->tempOwner != LOCPLINT->playerID)
+	// 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))
+	if(!LOCPLINT->localState->hasPath(hero))
 		return; // may happen when hero teleports
 
-	assert(hero == currentlyMovingHero);
 	assert(LOCPLINT->makingTurn);
-	assert(getNextTile(hero).has_value());
 
-	bool directlyAttackingCreature = details.attackedFrom.has_value() && getLastTile(hero) == details.attackedFrom;
+	bool directlyAttackingCreature = details.attackedFrom.has_value() && LOCPLINT->localState->getPath(hero).lastNode().coord == details.attackedFrom;
 
-	auto desiredTarget = getNextTile(hero);
-	auto actualTarget = hero->convertToVisitablePos(details.end);
+	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 (desiredTarget && heroChangedTile)
+	if(heroChangedTile)
 	{
-		if (*desiredTarget != actualTarget)
+		if(desiredTarget != actualTarget)
 		{
 			//invalidate path - movement was not along current path
 			//possible reasons: teleport, visit of object with "blocking visit" property
@@ -152,12 +141,20 @@ void HeroMovementController::updatePath(const CGHeroInstance * hero, const TryMo
 
 void HeroMovementController::heroMoved(const CGHeroInstance * hero, const TryMoveHero & details)
 {
-	if (details.result == TryMoveHero::EMBARK || details.result == TryMoveHero::DISEMBARK)
+	// 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)
@@ -167,61 +164,86 @@ void HeroMovementController::heroMoved(const CGHeroInstance * hero, const TryMov
 
 	updatePath(hero, details);
 
-	if(details.stopMovement()) //hero failed to move
+	if(details.stopMovement())
 	{
-		if (duringMovement)
+		if(duringMovement)
 			endHeroMove(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 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];
-//	}
+	// 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()
 {
-	//check if user cancelled movement
-	if (GH.input().ignoreEventsUntilInput())
+	// 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;
 
-	if (duringMovement)
-	{
-		assert(currentlyMovingHero);
-		auto const * hero = currentlyMovingHero;
+	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());
+	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)
-		{
-			endHeroMove(hero);
-		}
-		else if ( wantStop && canStop )
-		{
-			endHeroMove(hero);
-		}
-		else
-		{
-			moveHeroOnce(hero, LOCPLINT->localState->getPath(hero));
-		}
+	if(!canMove)
+	{
+		endHeroMove(hero);
+	}
+	else if(wantStop && canStop)
+	{
+		endHeroMove(hero);
+	}
+	else
+	{
+		moveHeroOnce(hero, LOCPLINT->localState->getPath(hero));
 	}
 }
 
@@ -234,6 +256,7 @@ void HeroMovementController::movementAbortRequested()
 void HeroMovementController::endHeroMove(const CGHeroInstance * hero)
 {
 	assert(duringMovement == true);
+	assert(currentlyMovingHero != nullptr);
 	duringMovement = false;
 	stoppingMovement = false;
 	currentlyMovingHero = nullptr;
@@ -244,17 +267,17 @@ void HeroMovementController::endHeroMove(const CGHeroInstance * hero)
 
 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)
+	if(moveType == EPathNodeAction::TELEPORT_BATTLE || moveType == EPathNodeAction::TELEPORT_BLOCKING_VISIT || moveType == EPathNodeAction::TELEPORT_NORMAL)
 		return {};
 
-	if (moveType == EPathNodeAction::EMBARK || moveType == EPathNodeAction::DISEMBARK)
+	if(moveType == EPathNodeAction::EMBARK || moveType == EPathNodeAction::DISEMBARK)
 		return {};
 
-	if (moveType == EPathNodeAction::BLOCKING_VISIT)
+	if(moveType == EPathNodeAction::BLOCKING_VISIT)
 		return {};
 
 	// flying movement sound
-	if (hero->hasBonusOfType(BonusType::FLYING_MOVEMENT))
+	if(hero->hasBonusOfType(BonusType::FLYING_MOVEMENT))
 		return AudioPath::builtin("HORSE10.wav");
 
 	auto prevTile = LOCPLINT->cb->getTile(posPrev);
@@ -264,7 +287,7 @@ AudioPath HeroMovementController::getMovementSoundFor(const CGHeroInstance * her
 	auto nextRoad = nextTile->roadType;
 	bool movingOnRoad = prevRoad->getId() != Road::NO_ROAD && nextRoad->getId() != Road::NO_ROAD;
 
-	if (movingOnRoad)
+	if(movingOnRoad)
 		return nextTile->terType->horseSound;
 	else
 		return nextTile->terType->horseSoundPenalty;
@@ -279,10 +302,10 @@ void HeroMovementController::updateMovementSound(const CGHeroInstance * h, int3
 	{
 		currentMovementSoundName = newSoundName;
 
-		if (currentMovementSoundChannel != -1)
+		if(currentMovementSoundChannel != -1)
 			CCS->soundh->stopSound(currentMovementSoundChannel);
 
-		if (!currentMovementSoundName.empty())
+		if(!currentMovementSoundName.empty())
 			currentMovementSoundChannel = CCS->soundh->playSound(currentMovementSoundName, -1, true);
 		else
 			currentMovementSoundChannel = -1;
@@ -291,17 +314,18 @@ void HeroMovementController::updateMovementSound(const CGHeroInstance * h, int3
 
 void HeroMovementController::stopMovementSound()
 {
-	CCS->soundh->stopSound(currentMovementSoundChannel);
+	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)
+	if(node.layer != EPathfindingLayer::LAND && node.layer != EPathfindingLayer::SAIL)
 		return false;
 
-	if (node.accessible != EPathAccessibility::ACCESSIBLE)
+	if(node.accessible != EPathAccessibility::ACCESSIBLE)
 		return false;
 
 	return true;
@@ -313,12 +337,15 @@ void HeroMovementController::movementStartRequested(const CGHeroInstance * h, co
 	duringMovement = true;
 	currentlyMovingHero = h;
 
-	CCS->curh->show();
+	CCS->curh->hide();
 	moveHeroOnce(h, path);
 }
 
 void HeroMovementController::moveHeroOnce(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();
@@ -329,9 +356,10 @@ void HeroMovementController::moveHeroOnce(const CGHeroInstance * h, const CGPath
 
 	int3 nextCoord = h->convertFromVisitablePos(nextNode.coord);
 
-	if (nextNode.isTeleportAction())
+	if(nextNode.isTeleportAction())
 	{
 		stopMovementSound();
+		logGlobal->trace("Requesting hero teleportation to %s", nextNode.coord.toString());
 		LOCPLINT->cb->moveHero(h, nextCoord, false);
 		return;
 	}

client/HeroMovementController.h

@@ -1,5 +1,5 @@
 /*
- * CPlayerInterface.h, part of VCMI engine
+ * HeroMovementController.h, part of VCMI engine
  *
  * Authors: listed in file AUTHORS in main folder
  *
@@ -32,15 +32,12 @@ class HeroMovementController
 	/// 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;
 
-	/// 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;
-
 	bool canHeroStopAtNode(const CGPathNode & node) const;
 
 	void updatePath(const CGHeroInstance * hero, const TryMoveHero & details);
@@ -56,11 +53,16 @@ class HeroMovementController
 
 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);

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
-	//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"))*/)
-	{
+	auto const complainRet = [&](const std::string & message){
 		//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 && !CGTeleport::isTeleport(t.topVisitableObj()))
+		complainRet("Hero cannot transit over this tile!");
 
 	//several generic blocks of code