Optimize pathfinding #4637

NRH-AA · 2024-03-23T08:27:08Z

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 · 2024-03-24T17:05:32Z

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.

src/map.cpp

NRH-AA · 2024-04-16T14:32:05Z

BUG: after changing the floor for the first time, monsters have a slow walk reaction. If the monsters had seen you before, they will have the correct walk reaction.

I am still looking into this but I seriously doubt this problem is created from this PR. The only reason you are noticing it more now is because monsters have faster reactions in 99% of cases compared to base repo

NRH-AA · 2024-04-19T05:15:17Z

The new constexpr update seems to have destroyed this

edit: I think changing int_fast32_t, ect is the problem.

NRH-AA · 2024-04-20T15:49:34Z

Can someone test the last commit. I think this should fix the pathfinding again. I also tried to fix the reaction speed when a monster first targets a player that has changed floors. @floki21uu

Keep in mind you will need to have updated to the new constexpr position methods.

NRH-AA · 2024-04-20T16:29:57Z

std::sqrt is running almost 3x faster than std::hypot

ranisalt · 2024-04-20T20:57:48Z

edit: I think changing int_fast32_t, ect is the problem.

FP math is orders of magnitude slower than integer math. I don't see why you changed the get*Cost functions to use doubles without changing how they work.

std::sqrt is running almost 3x faster than std::hypot

How did you measure it?

NRH-AA · 2024-04-21T00:40:14Z

edit: I think changing int_fast32_t, ect is the problem.

FP math is orders of magnitude slower than integer math. I don't see why you changed the get*Cost functions to use doubles without changing how they work.

std::sqrt is running almost 3x faster than std::hypot

How did you measure it?

I used time measurements strictly around the sqrt operation. I checked 1sqm-100sqm paths. I also created a small project just as a frame of reference using only the sqrt and hypot so there wasn't any additional bloatware. All instances were 2-3x faster from what I saw. I also tried different data types with integers giving the best results with sqrt being 3-4x faster. With that being said, I am not aware of tools, ect that can be used so its just based on time measurements. Someone with more experience may be able to look into it more.

As for what I changed to double I can revert some of the code back to using integers. I didn't realize I should leave out the heuristic from the g (walk) cost and place it in its own variable. However, I do think leaving the heuristic as a double is best as from everything I read about path finding the more precise the better for the heuristic. In other words I am still learning and did some things that weren't needed. I also didn't know it was done using order of magnitude which I am assuming is referencing the extra cost for fields and destroying creatures

5 Iterations

100k iterations

Edit: I have tried moving back to integers for the walk cost function and the result is that because of how big of a difference the integers are compared to the result of our heuristic the algorithm can no longer find the path properly. So keeping the walk cost as a float or double seems to be the best option and we will have to modify the extra costs to work properly assuming they dont. I do think it works as expected though because I have just divided the values by 10.

We also may be able to just *10 the dx and dy before running sqrt on it as well.

floki21uu · 2024-04-22T11:44:53Z

The new constexpr update seems to have destroyed this

edit: I think changing int_fast32_t, ect is the problem.

destroyed what?

NRH-AA · 2024-04-22T15:38:08Z

The new constexpr update seems to have destroyed this
edit: I think changing int_fast32_t, ect is the problem.

destroyed what?

The path finding stopped working with @ranisalt's constexpr position change. I did revert the datatype changes in this repo for pathmatching so I think it should be working properly again.

NRH-AA · 2024-04-25T17:32:15Z

Lets get this added peeps

Codinablack · 2024-05-09T13:10:08Z

When you made this using the constexpr positions you had problems. It seems there were problems with that commit that have since been fixed, have you tried using the current version of the constexpr positions?

NRH-AA · 2024-05-28T19:25:55Z

When you made this using the constexpr positions you had problems. It seems there were problems with that commit that have since been fixed, have you tried using the current version of the constexpr positions?

Yes, and the fixes did solve the problem. The system works as it is so I will push updates to get it ready to merge with tfs master when people have tested it and say it can be merged once its updated to lastest commits. I can't keep doing micro updates.

EvilHero90 · 2024-05-28T21:03:29Z

Feel free to update this, once 1.6 is released it'll be merged directly into 1.7 dev cycle

ranisalt · 2024-05-28T22:45:00Z

src/creature.cpp

+		return;
+	}
+
+	if (lastPathUpdate - OTSYS_TIME() > 0) {


Suggested change

if (lastPathUpdate - OTSYS_TIME() > 0) {

if (lastPathUpdate > OTSYS_TIME()) {

Shouldn't this be called nextPathUpdate? It's always set to a date in the future.

ranisalt · 2024-05-28T22:47:43Z

src/creature.cpp

+	}
+
+	std::list<Creature*> newList;
+	newList.resize(followedByCreatures.size());


Please do not use std::list, and the way you use it doesn't make much sense, you resize it adding empty values, push to it and then clear it and never use any intermediate state, looks like it could be completely removed

ranisalt · 2024-05-28T22:48:35Z

src/game.cpp

+			creature->forceUpdatePath();
+		}
+		++it;
+	}


Why not use a range for here?

ranisalt · 2024-05-28T22:53:13Z

src/map.cpp

-	return nullptr;
+	std::sort(nodes.begin(), nodes.end(), [](AStarNode* left, AStarNode* right) { return left->f > right->f; });
+	AStarNode* retNode = nodes.back();
+	nodes.pop_back();


You can use nth_element to partially sort, bubbling the best element to the end (using the same comparison function) so it only performs N operations rather than N log N

ranisalt · 2024-05-28T22:55:10Z

src/map.cpp

-	++closedNodes;
-}
+	uint16_t dx = std::abs(p1.x - p2.x);
+	uint16_t dy = std::abs(p1.y - p2.y);


You must either use getX and getY or static cast both to a signed integer, otherwise they will overflow since both numbers are unsigned.

ranisalt · 2024-05-28T22:56:53Z

src/map.h


-	static int_fast32_t getMapWalkCost(AStarNode* node, const Position& neighborPos);
-	static int_fast32_t getTileWalkCost(const Creature& creature, const Tile* tile);
+	static double calculateHeuristic(const Position& p1, const Position& p2);


This function does not need to be member of AStarNode, it can be a free function within map.cpp

NRH-AA added 8 commits March 23, 2024 01:00

Optimize pathfinding

990ba89

Minor fleeing monsters fix

00951d2

Fix whitespace and else brackets

89be1ad

More cleanup

82ad8c9

Cleanup

632d202

Remove uneeded variable name change

e7b5725

Cleanup

bc36d30

Move neighbor arrays back to map.cpp to fix unused variable failure

68841aa

EPuncker requested review from marksamman, kornholi, DSpeichert, ranisalt, djarek, nekiro and MillhioreBT March 23, 2024 15:49

Fix some compiling warnings

edc1746

Fix incorrect method call in game.cpp

2476334

This comment was marked as outdated.

Sign in to view

NRH-AA added 7 commits March 24, 2024 23:18

Update to new creature isDead method

aa4153a

Update to clang format

9f882ca

Update to clang format

8d50780

Fix maxSearchDist=0 searches and delayed reaction to onFollowCreature

b89076b

Fix fleeing monster randomly turning

f4163e5

Reduce memory consumption and update pathmatching algorithm

cab2968

Clang formatting

3834fd5

github-advanced-security bot found potential problems Mar 30, 2024

View reviewed changes

src/map.cpp Fixed Show fixed Hide fixed

NRH-AA added 3 commits March 30, 2024 13:32

Clang formatting

3009b52

Remove uneeded check on new nodes

57ce003

Normalize pathfinding algorithm

a68e1f5

ranisalt reviewed Apr 16, 2024

View reviewed changes

src/map.cpp Outdated Show resolved Hide resolved

Replace std::sqrt for std::hypot

dac4fc5

NRH-AA requested review from ranisalt and Codinablack April 16, 2024 15:20

NRH-AA added 2 commits April 18, 2024 21:34

Merge branch 'otland:master' into Optimize_Pathfinding

8327be7

Update to new constexpr position methods

a6b3f25

Revert int declarations in pathmatching

4d891e9

NRH-AA added 4 commits April 20, 2024 09:13

Change std::hypot for much faster std::sqrtf

60efba9

Formatting

c1d8530

Add math.h

13e9d66

Revert sqrtf and math.h

c3d86ca

EPuncker added the enhancement Increase or improvement in quality, value, or extent label Apr 21, 2024

Remove uneeded path call and add speed check

8efe712

Merge branch 'otland:master' into Optimize_Pathfinding

7f2f868

EvilHero90 added this to the 1.8 milestone May 28, 2024

EvilHero90 added needs-triage Needs testing with screenshot/video confirmation priority: high labels May 28, 2024

ranisalt requested changes May 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize pathfinding #4637

Optimize pathfinding #4637

NRH-AA commented Mar 23, 2024 •

edited

MillhioreBT commented Mar 24, 2024

This comment was marked as outdated.

NRH-AA commented Apr 16, 2024 •

edited

NRH-AA commented Apr 19, 2024 •

edited

NRH-AA commented Apr 20, 2024 •

edited

NRH-AA commented Apr 20, 2024 •

edited

ranisalt commented Apr 20, 2024

NRH-AA commented Apr 21, 2024 •

edited

floki21uu commented Apr 22, 2024

NRH-AA commented Apr 22, 2024

NRH-AA commented Apr 25, 2024

Codinablack commented May 9, 2024

NRH-AA commented May 28, 2024 •

edited

EvilHero90 commented May 28, 2024

ranisalt May 28, 2024

ranisalt May 28, 2024

ranisalt May 28, 2024

ranisalt May 28, 2024

ranisalt May 28, 2024

ranisalt May 28, 2024

	if (lastPathUpdate - OTSYS_TIME() > 0) {
	if (lastPathUpdate > OTSYS_TIME()) {

Optimize pathfinding #4637

Are you sure you want to change the base?

Optimize pathfinding #4637

Conversation

NRH-AA commented Mar 23, 2024 • edited

Pull Request Prelude

Changes Proposed

MillhioreBT commented Mar 24, 2024

This comment was marked as outdated.

NRH-AA commented Apr 16, 2024 • edited

NRH-AA commented Apr 19, 2024 • edited

NRH-AA commented Apr 20, 2024 • edited

NRH-AA commented Apr 20, 2024 • edited

ranisalt commented Apr 20, 2024

NRH-AA commented Apr 21, 2024 • edited

floki21uu commented Apr 22, 2024

NRH-AA commented Apr 22, 2024

NRH-AA commented Apr 25, 2024

Codinablack commented May 9, 2024

NRH-AA commented May 28, 2024 • edited

EvilHero90 commented May 28, 2024

ranisalt May 28, 2024

Choose a reason for hiding this comment

ranisalt May 28, 2024

Choose a reason for hiding this comment

ranisalt May 28, 2024

Choose a reason for hiding this comment

ranisalt May 28, 2024

Choose a reason for hiding this comment

ranisalt May 28, 2024

Choose a reason for hiding this comment

ranisalt May 28, 2024

Choose a reason for hiding this comment

NRH-AA commented Mar 23, 2024 •

edited

NRH-AA commented Apr 16, 2024 •

edited

NRH-AA commented Apr 19, 2024 •

edited

NRH-AA commented Apr 20, 2024 •

edited

NRH-AA commented Apr 20, 2024 •

edited

NRH-AA commented Apr 21, 2024 •

edited

NRH-AA commented May 28, 2024 •

edited