Merge pull request #1534 from kambala-decapitator/pathfinder-fly

fix movement cost with Fly

AI/Nullkiller/Pathfinding/AINodeStorage.cpp

@@ -285,7 +285,7 @@ std::vector<CGPathNode *> AINodeStorage::calculateNeighbours(
 
 	for(auto & neighbour : accessibleNeighbourTiles)
 	{
-		for(EPathfindingLayer i = EPathfindingLayer::LAND; i <= EPathfindingLayer::AIR; i.advance(1))
+		for(EPathfindingLayer i = EPathfindingLayer::LAND; i < EPathfindingLayer::NUM_LAYERS; i.advance(1))
 		{
 			auto nextNode = getOrCreateNode(neighbour, i, srcNode->actor);
 

AI/VCAI/Pathfinding/AINodeStorage.cpp

@@ -167,7 +167,7 @@ std::vector<CGPathNode *> AINodeStorage::calculateNeighbours(
 
 	for(auto & neighbour : accessibleNeighbourTiles)
 	{
-		for(EPathfindingLayer i = EPathfindingLayer::LAND; i <= EPathfindingLayer::AIR; i.advance(1))
+		for(EPathfindingLayer i = EPathfindingLayer::LAND; i < EPathfindingLayer::NUM_LAYERS; i.advance(1))
 		{
 			auto nextNode = getOrCreateNode(neighbour, i, srcNode->chainMask);
 

lib/CCreatureHandler.cpp

@@ -331,7 +331,7 @@ TerrainId CCreature::getNativeTerrain() const
 	static const auto selectorNoTerrainPenalty = Selector::type()(Bonus::NO_TERRAIN_PENALTY);
 
 	//this code is used in the CreatureTerrainLimiter::limit to setup battle bonuses
-	//and in the CGHeroInstance::getNativeTerrain() to setup mevement bonuses or/and penalties.
+	//and in the CGHeroInstance::getNativeTerrain() to setup movement bonuses or/and penalties.
 	return hasBonus(selectorNoTerrainPenalty, selectorNoTerrainPenalty)
 		? TerrainId(ETerrainId::ANY_TERRAIN)
 		: (*VLC->townh)[faction]->nativeTerrain;

lib/CCreatureHandler.h

@@ -161,9 +161,6 @@ public:
 	bool isItNativeTerrain(TerrainId terrain) const;
 	/**
 	Returns creature native terrain considering some terrain bonuses.
-	@param considerBonus is used to avoid Dead Lock when this method is called inside getAllBonuses
-	considerBonus = true is called from Pathfinder and fills actual nativeTerrain considering bonus(es).
-	considerBonus = false is called on Battle init and returns already prepared nativeTerrain without Bonus system calling.
 	*/
 	TerrainId getNativeTerrain() const;
 	int32_t getIndex() const override;

lib/CPathfinder.cpp

@@ -78,7 +78,7 @@ std::vector<CGPathNode *> NodeStorage::calculateNeighbours(
 
 	for(auto & neighbour : accessibleNeighbourTiles)
 	{
-		for(EPathfindingLayer i = EPathfindingLayer::LAND; i <= EPathfindingLayer::AIR; i.advance(1))
+		for(EPathfindingLayer i = EPathfindingLayer::LAND; i < EPathfindingLayer::NUM_LAYERS; i.advance(1))
 		{
 			auto * node = getNode(neighbour, i);
 
@@ -117,7 +117,7 @@ std::vector<CGPathNode *> NodeStorage::calculateTeleportations(
 std::vector<int3> CPathfinderHelper::getNeighbourTiles(const PathNodeInfo & source) const
 {
 	std::vector<int3> neighbourTiles;
-	neighbourTiles.reserve(16);
+	neighbourTiles.reserve(8);
 
 	getNeighbours(
 		*source.tile,
@@ -721,7 +721,7 @@ PathfinderBlockingRule::BlockingReason MovementAfterDestinationRule::getBlocking
 	switch(destination.action)
 	{
 	/// TODO: Investigate what kind of limitation is possible to apply on movement from visitable tiles
-	/// Likely in many cases we don't need to add visitable tile to queue when hero don't fly
+	/// Likely in many cases we don't need to add visitable tile to queue when hero doesn't fly
 	case CGPathNode::VISIT:
 	{
 		/// For now we only add visitable tile into queue when it's teleporter that allow transit
@@ -730,7 +730,7 @@ PathfinderBlockingRule::BlockingReason MovementAfterDestinationRule::getBlocking
 		if(pathfinderHelper->isAllowedTeleportEntrance(objTeleport))
 		{
 			/// For now we'll always allow transit over teleporters
-			/// Transit over whirlpools only allowed when hero protected
+			/// Transit over whirlpools only allowed when hero is protected
 			return BlockingReason::NONE;
 		}
 		else if(destination.nodeObject->ID == Obj::GARRISON
@@ -1163,8 +1163,8 @@ int CPathfinderHelper::getMaxMovePoints(const EPathfindingLayer & layer) const
 }
 
 void CPathfinderHelper::getNeighbours(
-	const TerrainTile & srct,
-	const int3 & tile,
+	const TerrainTile & srcTile,
+	const int3 & srcCoord,
 	std::vector<int3> & vec,
 	const boost::logic::tribool & onLand,
 	const bool limitCoastSailing) const
@@ -1179,35 +1179,32 @@ void CPathfinderHelper::getNeighbours(
 
 	for(const auto & dir : dirs)
 	{
-		const int3 hlp = tile + dir;
-		if(!map->isInTheMap(hlp))
+		const int3 destCoord = srcCoord + dir;
+		if(!map->isInTheMap(destCoord))
 			continue;
 
-		const TerrainTile & hlpt = map->getTile(hlp);
-		if(!hlpt.terType->isPassable())
+		const TerrainTile & destTile = map->getTile(destCoord);
+		if(!destTile.terType->isPassable())
 			continue;
 
 // 		//we cannot visit things from blocked tiles
-// 		if(srct.blocked && !srct.visitable && hlpt.visitable && srct.blockingObjects.front()->ID != HEROI_TYPE)
+// 		if(srcTile.blocked && !srcTile.visitable && destTile.visitable && srcTile.blockingObjects.front()->ID != HEROI_TYPE)
 // 		{
 // 			continue;
 // 		}
 
 		/// Following condition let us avoid diagonal movement over coast when sailing
-		if(srct.terType->isWater() && limitCoastSailing && hlpt.terType->isWater() && dir.x && dir.y) //diagonal move through water
+		if(srcTile.terType->isWater() && limitCoastSailing && destTile.terType->isWater() && dir.x && dir.y) //diagonal move through water
 		{
-			int3 hlp1 = tile;
-			int3 hlp2 = tile;
-			hlp1.x += dir.x;
-			hlp2.y += dir.y;
-
-			if(map->getTile(hlp1).terType->isLand() || map->getTile(hlp2).terType->isLand())
+			const int3 horizontalNeighbour = srcCoord + int3{dir.x, 0, 0};
+			const int3 verticalNeighbour = srcCoord + int3{0, dir.y, 0};
+			if(map->getTile(horizontalNeighbour).terType->isLand() || map->getTile(verticalNeighbour).terType->isLand())
 				continue;
 		}
 
-		if(indeterminate(onLand) || onLand == hlpt.terType->isLand())
+		if(indeterminate(onLand) || onLand == destTile.terType->isLand())
 		{
-			vec.push_back(hlp);
+			vec.push_back(destCoord);
 		}
 	}
 }
@@ -1218,7 +1215,9 @@ int CPathfinderHelper::getMovementCost(
 	const TerrainTile * ct,
 	const TerrainTile * dt,
 	const int remainingMovePoints,
-	const bool checkLast) const
+	const bool checkLast,
+	boost::logic::tribool isDstSailLayer,
+	boost::logic::tribool isDstWaterLayer) const
 {
 	if(src == dst) //same tile
 		return 0;
@@ -1231,48 +1230,49 @@ int CPathfinderHelper::getMovementCost(
 		dt = hero->cb->getTile(dst);
 	}
 
-	/// TODO: by the original game rules hero shouldn't be affected by terrain penalty while flying.
-	/// Also flying movement only has penalty when player moving over blocked tiles.
-	/// So if you only have base flying with 40% penalty you can still ignore terrain penalty while having zero flying penalty.
-	int ret = hero->getTileCost(*dt, *ct, ti);
-	/// Unfortunately this can't be implemented yet as server don't know when player flying and when he's not.
-	/// Difference in cost calculation on client and server is much worse than incorrect cost.
-	/// So this one is waiting till server going to use pathfinder rules for path validation.
+	bool isSailLayer;
+	if(indeterminate(isDstSailLayer))
+		isSailLayer = hero->boat != nullptr && dt->terType->isWater();
+	else
+		isSailLayer = static_cast<bool>(isDstSailLayer);
 
-	if(dt->blocked && ti->hasBonusOfType(Bonus::FLYING_MOVEMENT))
-	{
-		ret = static_cast<int>(ret * (100.0 + ti->valOfBonuses(Bonus::FLYING_MOVEMENT)) / 100.0);
-	}
-	else if(dt->terType->isWater())
+	bool isWaterLayer;
+	if(indeterminate(isDstWaterLayer))
+		isWaterLayer = dt->terType->isWater();
+	else
+		isWaterLayer = static_cast<bool>(isDstWaterLayer);
+
+	int ret = hero->getTileCost(*dt, *ct, ti);
+	if(isSailLayer)
 	{
-		if(hero->boat && ct->hasFavorableWinds() && dt->hasFavorableWinds())
-			ret = static_cast<int>(ret * 0.666);
-		else if(!hero->boat && ti->hasBonusOfType(Bonus::WATER_WALKING))
-		{
-			ret = static_cast<int>(ret * (100.0 + ti->valOfBonuses(Bonus::WATER_WALKING)) / 100.0);
-		}
+		if(ct->hasFavorableWinds())
+			ret = static_cast<int>(ret * 2.0 / 3);
 	}
+	else if(ti->hasBonusOfType(Bonus::FLYING_MOVEMENT))
+		vstd::amin(ret, GameConstants::BASE_MOVEMENT_COST + ti->valOfBonuses(Bonus::FLYING_MOVEMENT));
+	else if(isWaterLayer && ti->hasBonusOfType(Bonus::WATER_WALKING))
+		ret = static_cast<int>(ret * (100.0 + ti->valOfBonuses(Bonus::WATER_WALKING)) / 100.0);
 
 	if(src.x != dst.x && src.y != dst.y) //it's diagonal move
 	{
 		int old = ret;
 		ret = static_cast<int>(ret * M_SQRT2);
 		//diagonal move costs too much but normal move is possible - allow diagonal move for remaining move points
+		// https://heroes.thelazy.net/index.php/Movement#Diagonal_move_exception
 		if(ret > remainingMovePoints && remainingMovePoints >= old)
 		{
 			return remainingMovePoints;
 		}
 	}
 
-	/// TODO: This part need rework in order to work properly with flying and water walking
-	/// Currently it's only work properly for normal movement or sailing
-	int left = remainingMovePoints-ret;
-	if(checkLast && left > 0 && remainingMovePoints-ret < 250) //it might be the last tile - if no further move possible we take all move points
+	const int left = remainingMovePoints - ret;
+	constexpr auto maxCostOfOneStep = static_cast<int>(175 * M_SQRT2); // diagonal move on Swamp - 247 MP
+	if(checkLast && left > 0 && left <= maxCostOfOneStep) //it might be the last tile - if no further move possible we take all move points
 	{
 		std::vector<int3> vec;
 		vec.reserve(8); //optimization
 		getNeighbours(*dt, dst, vec, ct->terType->isLand(), true);
-		for(auto & elem : vec)
+		for(const auto & elem : vec)
 		{
 			int fcost = getMovementCost(dst, elem, nullptr, nullptr, left, false);
 			if(fcost <= left)

lib/CPathfinder.h

@@ -65,7 +65,7 @@ struct DLL_LINKAGE CGPathNode
 		VISITABLE, //tile can be entered as the last tile in path
 		BLOCKVIS,  //visitable from neighboring tile but not passable
 		FLYABLE, //can only be accessed in air layer
-		BLOCKED //tile can't be entered nor visited
+		BLOCKED //tile can be neither entered nor visited
 	};
 
 	CGPathNode * theNodeBefore;
@@ -580,8 +580,8 @@ public:
 	std::vector<int3> getTeleportExits(const PathNodeInfo & source) const;
 
 	void getNeighbours(
-		const TerrainTile & srct,
-		const int3 & tile,
+		const TerrainTile & srcTile,
+		const int3 & srcCoord,
 		std::vector<int3> & vec,
 		const boost::logic::tribool & onLand,
 		const bool limitCoastSailing) const;
@@ -591,8 +591,10 @@ public:
 		const int3 & dst,
 		const TerrainTile * ct,
 		const TerrainTile * dt,
-		const int remainingMovePoints =- 1,
-		const bool checkLast = true) const;
+		const int remainingMovePoints = -1,
+		const bool checkLast = true,
+		boost::logic::tribool isDstSailLayer = boost::logic::indeterminate,
+		boost::logic::tribool isDstWaterLayer = boost::logic::indeterminate) const;
 
 	int getMovementCost(
 		const PathNodeInfo & src,
@@ -606,7 +608,9 @@ public:
 			src.tile,
 			dst.tile,
 			remainingMovePoints,
-			checkLast
+			checkLast,
+			dst.node->layer == EPathfindingLayer::SAIL,
+			dst.node->layer == EPathfindingLayer::WATER
 		);
 	}
 

lib/mapObjects/CGHeroInstance.cpp

@@ -66,10 +66,10 @@ ui32 CGHeroInstance::getTileCost(const TerrainTile & dest, const TerrainTile & f
 {
 	int64_t ret = GameConstants::BASE_MOVEMENT_COST;
 
-	//if there is road both on dest and src tiles - use road movement cost
+	//if there is road both on dest and src tiles - use src road movement cost
 	if(dest.roadType->getId() != Road::NO_ROAD && from.roadType->getId() != Road::NO_ROAD)
 	{
-		ret = std::max(dest.roadType->movementCost, from.roadType->movementCost);
+		ret = from.roadType->movementCost;
 	}
 	else if(ti->nativeTerrain != from.terType->getId() &&//the terrain is not native
 			ti->nativeTerrain != ETerrainId::ANY_TERRAIN && //no special creature bonus
@@ -1106,22 +1106,19 @@ CBonusSystemNode & CGHeroInstance::whereShouldBeAttached(CGameState * gs)
 
 int CGHeroInstance::movementPointsAfterEmbark(int MPsBefore, int basicCost, bool disembark, const TurnInfo * ti) const
 {
-	int ret = 0; //take all MPs by default
-	bool localTi = false;
+	std::unique_ptr<TurnInfo> turnInfoLocal;
 	if(!ti)
 	{
-		localTi = true;
-		ti = new TurnInfo(this);
+		turnInfoLocal = std::make_unique<TurnInfo>(this);
+		ti = turnInfoLocal.get();
 	}
 
+	if(!ti->hasBonusOfType(Bonus::FREE_SHIP_BOARDING))
+		return 0; // take all MPs by default
+
 	int mp1 = ti->getMaxMovePoints(disembark ? EPathfindingLayer::LAND : EPathfindingLayer::SAIL);
 	int mp2 = ti->getMaxMovePoints(disembark ? EPathfindingLayer::SAIL : EPathfindingLayer::LAND);
-	if(ti->hasBonusOfType(Bonus::FREE_SHIP_BOARDING))
-		ret = static_cast<int>((MPsBefore - basicCost) * static_cast<double>(mp1) / mp2);
-
-	if(localTi)
-		delete ti;
-
+	int ret = static_cast<int>((MPsBefore - basicCost) * static_cast<double>(mp1) / mp2);
 	return ret;
 }
 

mapeditor/playerparams.cpp

@@ -59,7 +59,10 @@ PlayerParams::PlayerParams(MapController & ctrl, int playerId, QWidget *parent)
 		{
 			auto * ctown = town->town;
 			if(!ctown)
+			{
 				ctown = VLC->townh->randomTown;
+				town->town = ctown;
+			}
 			if(ctown && town->getOwner().getNum() == playerColor)
 			{
 				if(playerInfo.hasMainTown && playerInfo.posOfMainTown == town->pos)

server/CGameHandler.cpp

@@ -2342,8 +2342,6 @@ bool CGameHandler::moveHero(ObjectInstanceID hid, int3 dst, ui8 teleporting, boo
 
 	//check if destination tile is available
 	auto pathfinderHelper = std::make_unique<CPathfinderHelper>(gs, h, PathfinderOptions());
-
-	pathfinderHelper->updateTurnInfo(0);
 	auto ti = pathfinderHelper->getTurnInfo();
 
 	const bool canFly = pathfinderHelper->hasBonusOfType(Bonus::FLYING_MOVEMENT);