Optimize pathfinding

[NRH-AA]

Pull Request Prelude

• I have followed proper The Forgotten Server code styling.
• I have read and understood the contribution guidelines before making this PR.
• I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

This is a overhaul of when and how pathfinding is done. I did attempt this before but I closed the pull request due to congestion. I believe all problems from the original pull request have been fixed.

This will change the pathfinding to work as A* algorithm rather than djikstra's.
It will also add more pathfinding calls as needed fixing monster moving around slowly due to 1 second pathfinding interval.
This PR also includes separation of when creatures will update their path from creatures onThink.
Lastly it includes some fixes for bugs which were not detectable until pathfinding was able to be called more often.

Issues addressed:
Might fix: #4617

[MillhioreBT]

I did a quick test but this PR doesn't work, the monsters walk randomly and move away from the target slowly until they disappear from sight, and the summons don't have any type of movement.

[Codinablack]

Overall, I didn't see anything sticking out to me that looked like an obvious reason to hold back this PR... bearing that in mind, I also have not personally tested this code and various situations / scenarios either.

I have marked some areas I feel could use further optimization (if it would indeed provide such) by making some small easy changes. I also think some of the methods that were created during this PR could benefit from more passing by reference rather than by value or by pointer, but these are trivial things that are most likely optimized away anyways.

I give this a Passing Review because I see no validation in not approving this PR.

I am still curious about the suggestions I made during this review though :)

[NRH-AA]

LGTM

[ramon-bernardo]

I am analyzing the pathfinding code flow to understand its operation and direction. I realized that it would be more appropriate to force the inheriting classes to implement the goToFollowCreature method, instead of keeping this logic in the base class.

This not only improves code readability but also avoids responsibility inversion. The getMonster() method should be encapsulated within the Monster class, where decisions about monster behavior should be made, rather than in the base Creature class.

My suggestion is to ensure that goToFollowCreature is implemented in each derived class, respecting the principle that monster-specific logic should remain inside Monster, not in Creature. This eliminates the inversion of control flow and keeps responsibilities within the correct classes.

Check my fully review of goToFollowCreature, no more:

// Creature.h
Monster* monster = getMonster();
if (monster && !monster->getMaster() && (monster->isFleeing() || fpp.maxTargetDist > 1)) { ... }

Note: that I have also implemented this in Npc and Player.

Overview:

// Creature.h
virtual void goToFollowCreature();
// changed to
virtual void goToFollowCreature() = 0;

// removed, moved to goToFollowCreature in Monster.h
virtual void onFollowCreatureComplete(const Creature*) {}

void Npc::goToFollowCreature()
{
	if (followCreature) {
		FindPathParams fpp;
		getPathSearchParams(followCreature, fpp);
		listWalkDir.clear();
		if (getPathTo(followCreature->getPosition(), listWalkDir, fpp)) {
			hasFollowPath = true;
			startAutoWalk();
		} else {
			hasFollowPath = false;
		}
	}
}

void goToFollowCreature() override;

void Player::goToFollowCreature()
{
	// before
	Creature::goToFollowCreature();
	// after
	if (followCreature) {
		FindPathParams fpp;
		getPathSearchParams(followCreature, fpp);

		listWalkDir.clear();
		if (getPathTo(followCreature->getPosition(), listWalkDir, fpp)) {
			hasFollowPath = true;
			startAutoWalk();
		} else {
			hasFollowPath = false;
		}
	}
}

If the duplicated code is bothersome, feel free to move it to a new function in Creature, and call it in the Monster, Player and Npc class.

[Codinablack]

I am analyzing the pathfinding code flow to understand its operation and direction. I realized that it would be more appropriate to force the inheriting classes to implement the goToFollowCreature method, instead of keeping this logic in the base class.

This not only improves code readability but also avoids responsibility inversion. The getMonster() method should be encapsulated within the Monster class, where decisions about monster behavior should be made, rather than in the base Creature class.

My suggestion is to ensure that goToFollowCreature is implemented in each derived class, respecting the principle that monster-specific logic should remain inside Monster, not in Creature. This eliminates the inversion of control flow and keeps responsibilities within the correct classes.

Check my fully review of goToFollowCreature, no more:

// Creature.h
Monster* monster = getMonster();
if (monster && !monster->getMaster() && (monster->isFleeing() || fpp.maxTargetDist > 1)) { ... }

Note: that I have also implemented this in Npc and Player.

Overview:

// Creature.h
virtual void goToFollowCreature();
// changed to
virtual void goToFollowCreature() = 0;

// removed, moved to goToFollowCreature in Monster.h
virtual void onFollowCreatureComplete(const Creature*) {}

void Npc::goToFollowCreature()
{
	if (followCreature) {
		FindPathParams fpp;
		getPathSearchParams(followCreature, fpp);
		listWalkDir.clear();
		if (getPathTo(followCreature->getPosition(), listWalkDir, fpp)) {
			hasFollowPath = true;
			startAutoWalk();
		} else {
			hasFollowPath = false;
		}
	}
}

void goToFollowCreature() override;

void Player::goToFollowCreature()
{
	// before
	Creature::goToFollowCreature();
	// after
	if (followCreature) {
		FindPathParams fpp;
		getPathSearchParams(followCreature, fpp);

		listWalkDir.clear();
		if (getPathTo(followCreature->getPosition(), listWalkDir, fpp)) {
			hasFollowPath = true;
			startAutoWalk();
		} else {
			hasFollowPath = false;
		}
	}
}

If the duplicated code is bothersome, feel free to move it to a new function in Creature, and call it in the Monster, Player and Npc class.

I mean, this PR has been sitting here for ages, undergone MANY code reviews. I believe the issue you are raising, is one that is already currently present in the source code. I understand that this PR deals with that code, but can't your preferences on the control flow be set aside for the sake of merging this PR? You could easily take 10 minutes or less and make those changes to the codebase after this is merged. I mean, how long does it have to sit, how many revisions does it have to undergo before its finally merged?

[ramon-bernardo]

I am analyzing the pathfinding code flow to understand its operation and direction. I realized that it would be more appropriate to force the inheriting classes to implement the goToFollowCreature method, instead of keeping this logic in the base class.
This not only improves code readability but also avoids responsibility inversion. The getMonster() method should be encapsulated within the Monster class, where decisions about monster behavior should be made, rather than in the base Creature class.
My suggestion is to ensure that goToFollowCreature is implemented in each derived class, respecting the principle that monster-specific logic should remain inside Monster, not in Creature. This eliminates the inversion of control flow and keeps responsibilities within the correct classes.
Check my fully review of goToFollowCreature, no more:

// Creature.h
Monster* monster = getMonster();
if (monster && !monster->getMaster() && (monster->isFleeing() || fpp.maxTargetDist > 1)) { ... }

Note: that I have also implemented this in Npc and Player.
Overview:

// Creature.h
virtual void goToFollowCreature();
// changed to
virtual void goToFollowCreature() = 0;

// removed, moved to goToFollowCreature in Monster.h
virtual void onFollowCreatureComplete(const Creature*) {}

void Npc::goToFollowCreature()
{
	if (followCreature) {
		FindPathParams fpp;
		getPathSearchParams(followCreature, fpp);
		listWalkDir.clear();
		if (getPathTo(followCreature->getPosition(), listWalkDir, fpp)) {
			hasFollowPath = true;
			startAutoWalk();
		} else {
			hasFollowPath = false;
		}
	}
}

void goToFollowCreature() override;

void Player::goToFollowCreature()
{
	// before
	Creature::goToFollowCreature();
	// after
	if (followCreature) {
		FindPathParams fpp;
		getPathSearchParams(followCreature, fpp);

		listWalkDir.clear();
		if (getPathTo(followCreature->getPosition(), listWalkDir, fpp)) {
			hasFollowPath = true;
			startAutoWalk();
		} else {
			hasFollowPath = false;
		}
	}
}

If the duplicated code is bothersome, feel free to move it to a new function in Creature, and call it in the Monster, Player and Npc class.

I mean, this PR has been sitting here for ages, undergone MANY code reviews. I believe the issue you are raising, is one that is already currently present in the source code. I understand that this PR deals with that code, but can't your preferences on the control flow be set aside for the sake of merging this PR? You could easily take 10 minutes or less and make those changes to the codebase after this is merged. I mean, how long does it have to sit, how many revisions does it have to undergo before its finally merged?

Right, I send the #4811 with these improvements

[ramon-bernardo]

Rebase? @NRH-AA