Revamp MakeWay() and split it into CheckWay()/MakeWay()

[ell1e]

This change revamps MakeWay() with the following changes that are meant for allowing pathfinding and more:

1. There is now a separate CheckWay which allows to just check collision but without causing possible map or state changes. MakeWay will cause event updates sometimes, trying to move an event out of the way that blocks movement. This is undesirable for e.g. path finding wwhere just some hypothetical plan is being made and no movement or map changes intended yet.

2. There is now a separate CheckWayEx, which has the additional more complex options to either ignore all events and vehicles (and just check underlying map geometry for collision) and the option to specify a list of event IDs to specifically ignore for collision. This can be required for pathfinding.

   Elaborate reason: e.g. if you have two followers they need to ignore each other in pathing or in thin narrow corridors they'll block each others way, which will then cause them to search really elaborate and far routes around through e.g. entirely different corridors unless they ignore each other for the path planning (not the actual collision, just the planning).

3. There were two small behavior changes for MakeWay and MakeWayCollide: sometimes either would cause updates on an event that was deemed in the way when attempting a move, even though the map geometry would actually block that movement anyway. This seems like both unintended, and also not like someone would actually exploit that quirk for something useful, and also would have been harder to keep in that way than to just fix it.

I anticipate there might be some discussion if I did this split in a nice way. So therefore I decided that it might be better to pull request this in advance, before later actually trying to to pull request the path finding that makes use of all of this.

[ell1e]

Just to also put this here so it's easy to find: I agree with any relicensing by the maintainers as discussed here: #167 (comment)

[Ghabry]

Can you please change the NULL you added to nullptr?

tl;dr: I'm not fully certain yet if your change can break something. Will report back when I did more tests.

---

Stuff I have to check in RPG_RT vs. Player:

• Is the new WouldCollide(self, other, self_conflict) check before MakeWayUpdate a problem
• Is the earlier IsPassableTile call a problem

---

Thanks for this contribution. I will need to do some careful ingame testing to ensure that this does not break any behaviour. We must be careful to be RPG_RT (bug) compatible to not break any games. Unfortunately the games are pretty timing sensitive as the event execution often relies on race conditions.

To check if this has no side effects I manually inlined the function calls and looked if the result is the same as before (or if the new code has no side effects that matter).

Old code:

static bool MakeWayCollideEvent(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;
	}

	// Force the other event to update, allowing them to possibly move out of the way.
	MakeWayUpdate(other);

	if (!other.IsInPosition(x, y)) {
		return false;
	}

	return WouldCollide(self, other, self_conflict);
}

New code after inlining:

template <typename T>
static bool MakeWayCollideEvent(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;
	}

	if (!WouldCollide(self, other, self_conflict)) {
        	return false;
    	}

	// Let the event try to move away:
	MakeWayUpdate(other);

	if (!other.IsInPosition(x, y))
		return false;

	// Since it moved away, try again:
	if (&self == &other) {
		return false;
	}

	if (!other.IsInPosition(x, y)) {
		return false;
	}

	return WouldCollide(self, other, self_conflict);
}

Change here is

if (!WouldCollide(self, other, self_conflict)) {
    return false;
}

before MakeWayUpdate. This needs checking with RPG_RT if the preemption of the event on the target tile always happens or only if they would collide.

In MakeWay you moved IsPassableTile before the MakeWayUpdate logic. I think this could can break the code because a tile is also not passable when an event is standing on it. Then the code will exit early without attempting to do the MakeWay stuff.

[ell1e]

In MakeWay you moved IsPassableTile before the MakeWayUpdate logic. I think this could can break the code because a tile is also not passable when an event is standing on it.

After some research now, this is correct and the new order I did for this one is definitely broken. I didn't expect IsPassableTile to also check events, my bad. I'll fix it in a few minutes.

[ell1e]

Okay, so I revamped the use of IsPassableTile to hopefully correct the 2nd point. I tried to rethink as best as possible what order is both correct and maintainable in terms of how the code is split up and also what avoids running a larger amount of checks twice (for performance reasons), so hopefully I ended up with a reasonable result. Let me know what you think!

[ell1e]

I updated it to nullptr now. For what it's worth, some other unrelated spots could also use that change, but that seems like beyond the scope of this pull request so I left it alone

[Ghabry]

I think the adjusted MakeWay/CheckWay code will now work correctly.

But unfortunately I have to ask you to undo the change to MakeWayCollideEvent.

The problem is that your code added WouldCollide before MakeWayUpdate(other). This sounds sensible but is not how RPG_RT behaves: Events will preempt even when they are not colliding (so walking on an event on a different layer will preempt it). Changing this could break games as it alters the timing.

Just revert it to the old code instead of invoking CheckWayTestCollideEvent. I don't think it is worth here to add bool flags. The function is not very long.

Original code:

template <typename T>
static bool MakeWayCollideEvent(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;
	}

	// Force the other event to update, allowing them to possibly move out of the way.
	MakeWayUpdate(other);

	if (!other.IsInPosition(x, y)) {
		return false;
	}

	return WouldCollide(self, other, self_conflict);
}

[ell1e]

on it 👍 I understand the motivation, even though I find the old behavior a bit nonsensical. but i guess it's likely true that it might have corner case timing changes