Fix regression of MakeWay() from MakeWay()/CheckWay() split

[ell1e]

This should fix the regression I introduced in #3120 into MakeWay, where some corner cases no longer call MakeWayCollide like before which breaks some movement routes that disable collision dynamically. This fix also removes the unnecessary CheckWayEx functions of GameMap and GameCharacter again, and just adds multiple signatures for CheckWay. It also removes all the out_... pointers since I managed to merge the core functionality of MakeWay and CheckWay once more, making this whole pointer passing around unnecessary.

Sorry again for the regression. I hope this one will fare better, I compared it directly against the original MakeWay before all my changes and hopefully this time, no unintended change slipped by.

Fixes #3129 (at least in my tests)

[ell1e]

Because it's easier to understand in some ways, here is a diff of the fixed version (the code in this pull request) compared to the working original (before the two pull requests, before the breakage #3120 & this fix) of src/game_map.cpp. That should help with reasoning over whether this fixed version is actually correct:

$ diff -u --ignore-all-space EasyRPG-old/src/game_map.cpp EasyRPG-Player/src/game_map.cpp 
--- EasyRPG-old/src/game_map.cpp	2023-10-24 15:20:41.474368139 +0200
+++ EasyRPG-Player/src/game_map.cpp	2023-10-26 04:30:05.643090921 +0200
@@ -22,6 +22,7 @@
 #include <algorithm>
 #include <climits>
 #include <numeric>
+#include <unordered_set>
 
 #include "async_handler.h"
 #include "options.h"
@@ -533,6 +534,19 @@
 }
 
 template <typename T>
+static bool CheckWayTestCollideEvent(int x, int y, const Game_Character& self, T& other, bool self_conflict) {
+	if (&self == &other) {
+		return false;
+	}
+
+	if (!other.IsInPosition(x, y)) {
+		return false;
+	}
+
+	return WouldCollide(self, other, self_conflict);
+}
+
+template <typename T>
 static bool MakeWayCollideEvent(int x, int y, const Game_Character& self, T& other, bool self_conflict) {
 	if (&self == &other) {
 		return false;
@@ -559,14 +573,39 @@
 	return Game_Vehicle::None;
 }
 
-bool Game_Map::MakeWay(const Game_Character& self,
+bool Game_Map::CheckWay(const Game_Character& self,
 		int from_x, int from_y,
 		int to_x, int to_y
 		)
 {
+	return CheckOrMakeWayEx(
+		self, from_x, from_y, to_x, to_y, true, nullptr, false
+	);
+}
+
+bool Game_Map::CheckWay(const Game_Character& self,
+		int from_x, int from_y,
+		int to_x, int to_y,
+		bool check_events_and_vehicles,
+		std::unordered_set<int> *ignore_some_events_by_id) {
+	return CheckOrMakeWayEx(
+		self, from_x, from_y, to_x, to_y,
+		check_events_and_vehicles,
+		ignore_some_events_by_id, false
+	);
+}
+
+bool Game_Map::CheckOrMakeWayEx(const Game_Character& self,
+		int from_x, int from_y,
+		int to_x, int to_y,
+		bool check_events_and_vehicles,
+		std::unordered_set<int> *ignore_some_events_by_id,
+		bool make_way
+		)
+{
 	// Infer directions before we do any rounding.
-	const auto bit_from = GetPassableMask(from_x, from_y, to_x, to_y);
-	const auto bit_to = GetPassableMask(to_x, to_y, from_x, from_y);
+	const int bit_from = GetPassableMask(from_x, from_y, to_x, to_y);
+	const int bit_to = GetPassableMask(to_x, to_y, from_x, from_y);
 
 	// Now round for looping maps.
 	to_x = Game_Map::RoundX(to_x);
@@ -582,8 +621,20 @@
 	}
 
 	const auto vehicle_type = GetCollisionVehicleType(&self);
-
 	bool self_conflict = false;
+
+	// Depending on whether we're supposed to call MakeWayCollideEvent
+	// (which might change the map) or not, choose what to call:
+	auto CheckOrMakeCollideEvent = [&](auto& other) {
+		if (make_way) {
+			return MakeWayCollideEvent(to_x, to_y, self, other, self_conflict);
+		} else {
+			return CheckWayTestCollideEvent(
+				to_x, to_y, self, other, self_conflict
+			);
+		}
+	};
+
 	if (!self.IsJumping()) {
 		// Check for self conflict.
 		// If this event has a tile graphic and the tile itself has passage blocked in the direction
@@ -611,44 +662,59 @@
 			}
 		}
 	}
-
-	if (vehicle_type != Game_Vehicle::Airship) {
+	if (vehicle_type != Game_Vehicle::Airship && check_events_and_vehicles) {
 		// Check for collision with events on the target tile.
 		for (auto& other: GetEvents()) {
-			if (MakeWayCollideEvent(to_x, to_y, self, other, self_conflict)) {
+			if (ignore_some_events_by_id != NULL &&
+					ignore_some_events_by_id->find(other.GetId()) !=
+					ignore_some_events_by_id->end())
+				continue;
+			if (CheckOrMakeCollideEvent(other)) {
 				return false;
 			}
 		}
 		auto& player = Main_Data::game_player;
 		if (player->GetVehicleType() == Game_Vehicle::None) {
-			if (MakeWayCollideEvent(to_x, to_y, self, *Main_Data::game_player, self_conflict)) {
+			if (CheckOrMakeCollideEvent(*Main_Data::game_player)) {
 				return false;
 			}
 		}
 		for (auto vid: { Game_Vehicle::Boat, Game_Vehicle::Ship}) {
 			auto& other = vehicles[vid - 1];
 			if (other.IsInCurrentMap()) {
-				if (MakeWayCollideEvent(to_x, to_y, self, other, self_conflict)) {
+				if (CheckOrMakeCollideEvent(other)) {
 					return false;
 				}
 			}
 		}
 		auto& airship = vehicles[Game_Vehicle::Airship - 1];
 		if (airship.IsInCurrentMap() && self.GetType() != Game_Character::Player) {
-			if (MakeWayCollideEvent(to_x, to_y, self, airship, self_conflict)) {
+			if (CheckOrMakeCollideEvent(airship)) {
 				return false;
 			}
 		}
 	}
-
 	int bit = bit_to;
 	if (self.IsJumping()) {
 		bit = Passable::Down | Passable::Up | Passable::Left | Passable::Right;
 	}
 
-	return IsPassableTile(&self, bit, to_x, to_y);
+	return IsPassableTile(
+		&self, bit, to_x, to_y, check_events_and_vehicles, true
+		);
+}
+
+bool Game_Map::MakeWay(const Game_Character& self,
+		int from_x, int from_y,
+		int to_x, int to_y
+		)
+{
+	return CheckOrMakeWayEx(
+		self, from_x, from_y, to_x, to_y, true, NULL, true
+		);
 }
 
+
 bool Game_Map::CanLandAirship(int x, int y) {
 	if (!Game_Map::IsValid(x, y)) return false;
 
@@ -742,11 +808,22 @@
 	return (passages_down[tile_id] & bit) != 0;
 }
 
-bool Game_Map::IsPassableTile(const Game_Character* self, int bit, int x, int y) {
+bool Game_Map::IsPassableTile(
+		const Game_Character* self, int bit, int x, int y
+		) {
+	return IsPassableTile(
+		self, bit, x, y, true, true
+	);
+}
+
+bool Game_Map::IsPassableTile(
+		const Game_Character* self, int bit, int x, int y,
+		bool check_events_and_vehicles, bool check_map_geometry
+		) {
 	if (!IsValid(x, y)) return false;
 
 	const auto vehicle_type = GetCollisionVehicleType(self);
-
+	if (check_events_and_vehicles) {
 	if (vehicle_type != Game_Vehicle::None) {
 		const auto* terrain = lcf::ReaderUtil::GetElement(lcf::Data::terrains, GetTerrainTag(x, y));
 		if (!terrain) {
@@ -795,7 +872,9 @@
 				break;
 		};
 	}
+	}
 
+	if (check_map_geometry) {
 	int tile_index = x + y * GetTilesX();
 	int tile_id = map->upper_layer[tile_index] - BLOCK_F;
 	tile_id = map_info.upper_tiles[tile_id];
@@ -813,6 +892,9 @@
 		return true;
 
 	return IsPassableLowerTile(bit, tile_index);
+	} else {
+		return true;
+	}
 }
 
 int Game_Map::GetBushDepth(int x, int y) {

[Ghabry]

An alternative to the template is a lambda function. I was a bit uncreative with the name fn. maybe you have a better one 😅

Except for other you always pass in the same arguments so I prebound them by reference (this way you can alter e.g. self_reference and it will have the same value in the lambda. magical.

diff --git a/src/game_map.cpp b/src/game_map.cpp
index 54a13ce4..2971d091 100644
--- a/src/game_map.cpp
+++ b/src/game_map.cpp
@@ -595,19 +595,6 @@ bool Game_Map::CheckWay(const Game_Character& self,
 	);
 }
 
-struct MakeWayCollideSwitchCaller {
-	bool make_way;
-	template <typename T>
-	bool Call(
-			int x, int y, const Game_Character& self, T& other,
-			bool self_conflict
-			) {
-		if (make_way)
-			return MakeWayCollideEvent(x, y, self, other, self_conflict);
-		return CheckWayTestCollideEvent(x, y, self, other, self_conflict);
-	}
-};
-
 bool Game_Map::CheckWayOrMakeEx(const Game_Character& self,
 		int from_x, int from_y,
 		int to_x, int to_y,
@@ -616,11 +603,6 @@ bool Game_Map::CheckWayOrMakeEx(const Game_Character& self,
 		std::unordered_set<int> *ignore_some_events_by_id
 		)
 {
-	// Depending on whether we're supposed to call MakeWayCollideEvent
-	// (which might change the map) or not, choose what to call:
-	MakeWayCollideSwitchCaller caller;
-	caller.make_way = make_way;
-
 	// Infer directions before we do any rounding.
 	const int bit_from = GetPassableMask(from_x, from_y, to_x, to_y);
 	const int bit_to = GetPassableMask(to_x, to_y, from_x, from_y);
@@ -640,6 +622,17 @@ bool Game_Map::CheckWayOrMakeEx(const Game_Character& self,
 
 	const auto vehicle_type = GetCollisionVehicleType(&self);
 	bool self_conflict = false;
+
+	// Depending on whether we're supposed to call MakeWayCollideEvent
+	// (which might change the map) or not, choose what to call:
+	auto fn = [&](auto& other) {
+		if (make_way) {
+			return MakeWayCollideEvent(to_x, to_y, self, other, self_conflict);
+		} else {
+			return CheckWayTestCollideEvent(to_x, to_y, self, other, self_conflict);
+		}
+	};
+
 	if (!self.IsJumping()) {
 		// Check for self conflict.
 		// If this event has a tile graphic and the tile itself has passage blocked in the direction
@@ -674,27 +667,27 @@ bool Game_Map::CheckWayOrMakeEx(const Game_Character& self,
 					ignore_some_events_by_id->find(other.GetId()) !=
 					ignore_some_events_by_id->end())
 				continue;
-			if (caller.Call(to_x, to_y, self, other, self_conflict)) {
+			if (fn(other)) {
 				return false;
 			}
 		}
 		auto& player = Main_Data::game_player;
 		if (player->GetVehicleType() == Game_Vehicle::None) {
-			if (caller.Call(to_x, to_y, self, *Main_Data::game_player, self_conflict)) {
+			if (fn(*Main_Data::game_player)) {
 				return false;
 			}
 		}
 		for (auto vid: { Game_Vehicle::Boat, Game_Vehicle::Ship}) {
 			auto& other = vehicles[vid - 1];
 			if (other.IsInCurrentMap()) {
-				if (caller.Call(to_x, to_y, self, other, self_conflict)) {
+				if (fn(other)) {
 					return false;
 				}
 			}
 		}
 		auto& airship = vehicles[Game_Vehicle::Airship - 1];
 		if (airship.IsInCurrentMap() && self.GetType() != Game_Character::Player) {
-			if (caller.Call(to_x, to_y, self, airship, self_conflict)) {
+			if (fn(airship)) {
 				return false;
 			}
 		}

[ell1e]

Thanks for the idea, I incorporated the lambda variant now! It's also incorporated into the diff above.

[Ghabry]

I approve this. The new version is much cleaner. I would still suggest some minor further improvements:

1. Because one function is called MakeWayCollideEvent the other could be called CheckWayCollideEvent (without the Test)
2. CheckWayOrMakeEx -> CheckOrMakeWayEx 😅

CheckOrMakeWayEx(const Game_Character& self,
		int from_x, int from_y,
		int to_x, int to_y,
		bool ignore_events_and_vehicles,
		bool make_way, // <- move to the end
		std::unordered_set<int> *ignore_some_events_by_id
)

// Different argument order compared to
bool Game_Map::CheckWay(const Game_Character& self,
		int from_x, int from_y,
		int to_x, int to_y,
		bool ignore_events_and_vehicles,
		std::unordered_set<int> *ignore_some_events_by_id)
}

The bool make_way should be the last argument to avoid bugs in the future.

4. Rename ignore_events_and_vehicles to check_events_and_vehicles and flip the logic. Otherwise true stands for something negative. (ignore_some_events_by_id can stay as there is no simple way to flip it.)

[ell1e]

Otherwise true stands for something negative

For what it's worth, I usually use "true" as non-default or special behavior enabled, and "false" as default standard behavior. But I'm happy to go with whatever convention you prefer.

Because one function is called MakeWayCollideEvent the other could be called CheckWayCollideEvent (without the Test)

I left this one as it is for now, because MakeWayCollideEvent actually prompts a potential dodge and collision reaction, while the check way variant doesn't, hence "CollideEventTest" Edit: since it only tests or simulates but doesn't execute the effects, if you will. Open for better naming ideas!

[Ghabry]

Jenkins: Test this please