Fix #12228, Fix #12231: CheckShipReverse only restricts path when it has to

src/pathfinder/yapf/yapf_ship.cpp

@@ -172,6 +172,14 @@ public:
 		return 'w';
 	}
 
+	/** Returns a random trackdir out of a set of trackdirs. */
+	static Trackdir GetRandomTrackdir(TrackdirBits trackdirs)
+	{
+		const int strip_amount = RandomRange(CountBits(trackdirs));
+		for (int s = 0; s < strip_amount; ++s) RemoveFirstTrackdir(&trackdirs);
+		return FindFirstTrackdir(trackdirs);
+	}
+
 	/** Returns a random tile/trackdir that can be reached from the current tile/trackdir, or tile/INVALID_TRACK if none is available. */
 	static std::pair<TileIndex, Trackdir> GetRandomFollowUpTileTrackdir(const Ship *v, TileIndex tile, Trackdir dir)
 	{
@@ -180,17 +188,15 @@ public:
 			TrackdirBits dirs = follower.m_new_td_bits;
 			const TrackdirBits dirs_without_90_degree = dirs & ~TrackdirCrossesTrackdirs(dir);
 			if (dirs_without_90_degree != TRACKDIR_BIT_NONE) dirs = dirs_without_90_degree;
-			const int strip_amount = RandomRange(CountBits(dirs));
+			return { follower.m_new_tile, GetRandomTrackdir(dirs) };
-			for (int s = 0; s < strip_amount; ++s) RemoveFirstTrackdir(&dirs);
-			return { follower.m_new_tile, FindFirstTrackdir(dirs) };
 		}
 		return { follower.m_new_tile, INVALID_TRACKDIR };
 	}
 
 	/** Creates a random path, avoids 90 degree turns. */
-	static Trackdir CreateRandomPath(const Ship *v, Trackdir dir, ShipPathCache &path_cache, int path_length)
+	static Trackdir CreateRandomPath(const Ship *v, ShipPathCache &path_cache, int path_length)
 	{
-		std::pair<TileIndex, Trackdir> tile_dir = { v->tile, dir };
+		std::pair<TileIndex, Trackdir> tile_dir = { v->tile, v->GetVehicleTrackdir()};
 		for (int i = 0; i < path_length; ++i) {
 			tile_dir = GetRandomFollowUpTileTrackdir(v, tile_dir.first, tile_dir.second);
 			if (tile_dir.second == INVALID_TRACKDIR) break;
@@ -204,19 +210,14 @@ public:
 		return result;
 	}
 
-	static Trackdir ChooseShipTrack(const Ship *v, TileIndex tile, bool &path_found, ShipPathCache &path_cache)
+	static Trackdir ChooseShipTrack(const Ship *v, TileIndex tile, TrackdirBits forward_dirs, TrackdirBits reverse_dirs,
+		bool &path_found, ShipPathCache &path_cache, Trackdir &best_origin_dir)
 	{
-		const Trackdir trackdir = v->GetVehicleTrackdir();
-		assert(IsValidTrackdir(trackdir));
-
-		/* Convert origin trackdir to TrackdirBits. */
-		const TrackdirBits trackdirs = TrackdirToTrackdirBits(trackdir);
-
 		const std::vector<WaterRegionPatchDesc> high_level_path = YapfShipFindWaterRegionPath(v, tile, NUMBER_OR_WATER_REGIONS_LOOKAHEAD + 1);
 		if (high_level_path.empty()) {
 			path_found = false;
 			/* Make the ship move around aimlessly. This prevents repeated pathfinder calls and clearly indicates that the ship is lost. */
-			return CreateRandomPath(v, trackdir, path_cache, SHIP_LOST_PATH_LENGTH);
+			return CreateRandomPath(v, path_cache, SHIP_LOST_PATH_LENGTH);
 		}
 
 		/* Try one time without restricting the search area, which generally results in better and more natural looking paths.
@@ -226,7 +227,7 @@ public:
 			Tpf pf(MAX_SHIP_PF_NODES);
 
 			/* Set origin and destination nodes */
-			pf.SetOrigin(v->tile, trackdirs);
+			pf.SetOrigin(v->tile, forward_dirs | reverse_dirs);
 			pf.SetDestination(v);
 			const bool is_intermediate_destination = static_cast<int>(high_level_path.size()) >= NUMBER_OR_WATER_REGIONS_LOOKAHEAD + 1;
 			if (is_intermediate_destination) pf.SetIntermediateDestination(high_level_path.back());
@@ -239,14 +240,16 @@ public:
 			path_found = pf.FindPath(v);
 			Node *node = pf.GetBestNode();
 			if (attempt == 0 && !path_found) continue; // Try again with restricted search area.
-			if (!path_found || node == nullptr) return GetRandomFollowUpTileTrackdir(v, v->tile, trackdir).second;
+
+			/* Make the ship move around aimlessly. This prevents repeated pathfinder calls and clearly indicates that the ship is lost. */
+			if (!path_found) return CreateRandomPath(v, path_cache, SHIP_LOST_PATH_LENGTH);
 
 			/* Return only the path within the current water region if an intermediate destination was returned. If not, cache the entire path
 			 * to the final destination tile. The low-level pathfinder might actually prefer a different docking tile in a nearby region. Without
 			 * caching the full path the ship can get stuck in a loop. */
 			const WaterRegionPatchDesc end_water_patch = GetWaterRegionPatchInfo(node->GetTile());
-			const WaterRegionPatchDesc start_water_patch = GetWaterRegionPatchInfo(tile);
+			assert(GetWaterRegionPatchInfo(tile) == high_level_path.front());
-			assert(start_water_patch == high_level_path.front());
+			const WaterRegionPatchDesc start_water_patch = high_level_path.front();
 			while (node->m_parent) {
 				const WaterRegionPatchDesc node_water_patch = GetWaterRegionPatchInfo(node->GetTile());
 
@@ -262,10 +265,18 @@ public:
 				}
 				node = node->m_parent;
 			}
+			assert(node->GetTile() == v->tile);
+
+			/* Return INVALID_TRACKDIR to trigger a ship reversal if that is the best option. */
+			best_origin_dir = node->GetTrackdir();
+			if ((TrackdirToTrackdirBits(best_origin_dir) & forward_dirs) == TRACKDIR_BIT_NONE) {
+				path_cache.clear();
+				return INVALID_TRACKDIR;
+			}
 
 			/* A empty path means we are already at the destination. The pathfinder shouldn't have been called at all.
 			 * Return a random reachable trackdir to hopefully nudge the ship out of this strange situation. */
-			if (path_cache.empty()) return GetRandomFollowUpTileTrackdir(v, v->tile, trackdir).second;
+			if (path_cache.empty()) return CreateRandomPath(v, path_cache, 1);
 
 			/* Take out the last trackdir as the result. */
 			const Trackdir result = path_cache.front();
@@ -284,52 +295,30 @@ public:
 	 * Check whether a ship should reverse to reach its destination.
 	 * Called when leaving depot.
 	 * @param v Ship.
-	 * @param tile Current position.
-	 * @param td1 Forward direction.
-	 * @param td2 Reverse direction.
 	 * @param trackdir [out] the best of all possible reversed trackdirs.
 	 * @return true if the reverse direction is better.
 	 */
-	static bool CheckShipReverse(const Ship *v, TileIndex tile, Trackdir td1, Trackdir td2, Trackdir *trackdir)
+	static bool CheckShipReverse(const Ship *v, Trackdir *trackdir)
 	{
-		const std::vector<WaterRegionPatchDesc> high_level_path = YapfShipFindWaterRegionPath(v, tile, NUMBER_OR_WATER_REGIONS_LOOKAHEAD + 1);
+		bool path_found = false;
-		if (high_level_path.empty()) {
+		ShipPathCache dummy_cache;
-			if (trackdir) *trackdir = INVALID_TRACKDIR;
+		Trackdir best_origin_dir = INVALID_TRACKDIR;
-			return false;
-		}
 
-		/* Create pathfinder instance. */
-		Tpf pf(MAX_SHIP_PF_NODES);
-		/* Set origin and destination nodes. */
 		if (trackdir == nullptr) {
-			pf.SetOrigin(tile, TrackdirToTrackdirBits(td1) | TrackdirToTrackdirBits(td2));
+			/* The normal case, typically called when ships leave a dock. */
+			const Trackdir reverse_dir = ReverseTrackdir(v->GetVehicleTrackdir());
+			const TrackdirBits forward_dirs = TrackdirToTrackdirBits(v->GetVehicleTrackdir());
+			const TrackdirBits reverse_dirs = TrackdirToTrackdirBits(reverse_dir);
+			(void)ChooseShipTrack(v, v->tile, forward_dirs, reverse_dirs, path_found, dummy_cache, best_origin_dir);
+			return path_found && best_origin_dir == reverse_dir;
 		} else {
-			DiagDirection entry = ReverseDiagDir(VehicleExitDir(v->direction, v->state));
+			/* This gets called when a ship suddenly can't move forward, e.g. due to terraforming. */
-			TrackdirBits rtds = DiagdirReachesTrackdirs(entry) & TrackStatusToTrackdirBits(GetTileTrackStatus(tile, TRANSPORT_WATER, 0, entry));
+			const DiagDirection entry = ReverseDiagDir(VehicleExitDir(v->direction, v->state));
-			pf.SetOrigin(tile, rtds);
+			const TrackdirBits reverse_dirs = DiagdirReachesTrackdirs(entry) & TrackStatusToTrackdirBits(GetTileTrackStatus(v->tile, TRANSPORT_WATER, 0, entry));
+			(void)ChooseShipTrack(v, v->tile, TRACKDIR_BIT_NONE, reverse_dirs, path_found, dummy_cache, best_origin_dir);
+			*trackdir = path_found && best_origin_dir != INVALID_TRACKDIR ? best_origin_dir : GetRandomTrackdir(reverse_dirs);
+			return true;
 		}
-		pf.SetDestination(v);
-		if (high_level_path.size() > 1) pf.SetIntermediateDestination(high_level_path.back());
-		pf.RestrictSearch(high_level_path);
-
-		/* Find best path. */
-		if (!pf.FindPath(v)) return false;
-
-		Node *pNode = pf.GetBestNode();
-		if (pNode == nullptr) return false;
-
-		/* Path was found, walk through the path back to the origin. */
-		while (pNode->m_parent != nullptr) {
-			pNode = pNode->m_parent;
-		}
-
-		Trackdir best_trackdir = pNode->GetTrackdir();
-		if (trackdir != nullptr) {
-			*trackdir = best_trackdir;
-		} else {
-			assert(best_trackdir == td1 || best_trackdir == td2);
-		}
-		return best_trackdir != td1;
 	}
 };
 
@@ -437,14 +426,13 @@ struct CYapfShip : CYapfT<CYapfShip_TypesT<CYapfShip, CFollowTrackWater, CShipNo
 /** Ship controller helper - path finder invoker. */
 Track YapfShipChooseTrack(const Ship *v, TileIndex tile, bool &path_found, ShipPathCache &path_cache)
 {
-	Trackdir td_ret = CYapfShip::ChooseShipTrack(v, tile, path_found, path_cache);
+	Trackdir best_origin_dir = INVALID_TRACKDIR;
+	const TrackdirBits origin_dirs = TrackdirToTrackdirBits(v->GetVehicleTrackdir());
+	const Trackdir td_ret = CYapfShip::ChooseShipTrack(v, tile, origin_dirs, TRACKDIR_BIT_NONE, path_found, path_cache, best_origin_dir);
 	return (td_ret != INVALID_TRACKDIR) ? TrackdirToTrack(td_ret) : INVALID_TRACK;
 }
 
 bool YapfShipCheckReverse(const Ship *v, Trackdir *trackdir)
 {
-	Trackdir td = v->GetVehicleTrackdir();
+	return CYapfShip::CheckShipReverse(v, trackdir);
-	Trackdir td_rev = ReverseTrackdir(td);
-	TileIndex tile = v->tile;
-	return CYapfShip::CheckShipReverse(v, tile, td, td_rev, trackdir);
 }