Skip to content

Conversation

@sparr
Copy link
Member

@sparr sparr commented Aug 24, 2025

Summary

Bugfixes "Fix pathfinder zigzagging"

Purpose of change

When traveling orthogonally with a diagonal move at one end, the pathfinder will make a zigzag path. #82058 reported this and was closed when the vehicle diagonal pathfinding change was reverted, but the problem persists for foot travel.

Describe the solution

Undo the changes from #77556 that enabled pathfinding along map edges. Those changes improved pathfinding in rare edge cases, but also made it more expensive and introduced complexity that makes solving problems like this much harder, and would also have complicated upcoming efforts to make pathfinding compatible with highways.

Describe alternatives you've considered

See Discord discussion below https://discord.com/channels/598523535169945603/598529174302490644/1412437256802930789 and comments on this PR for description of some prior attempted solutions.

Testing

Did some overmap fast travel in game, confirmed paths generally look as expected and don't zig-zag.

Additional context

The pathfinder pretends that you can travel between vertices of the submap grid. This is generally a good abstraction, despite the player not actually occupying those locations but instead a tile adjacent to those locations. The abstraction breaks down when the pathfinder wants to travel along the edge of an OMT, thinking that there is no difference in cost between the two sides of that edge. A previous effort toward a solution was to add a penalty weight to the zigzag pattern, but this did not work reliably due to other complexities.

Also the pathfinder doesn't account for the cost of crossing half of the final map to reach its center, resulting in it scoring reaching the corner of that map as equivalent to reaching its edge. This causes the pathfinder to often prioritize traveling along the edges of a line of maps, which causes a zigzag.

I also added some new pathfinding test cases and additional checks and comments in existing test cases.

@github-actions github-actions bot added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` labels Aug 24, 2025
Copy link
Member

@kevingranade kevingranade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is entirely too magical a fix. That means that there is something else going on and you are likely adding a bug elsewhere.

@sparr
Copy link
Member Author

sparr commented Aug 24, 2025

Figuring out the need for the +1 felt like an excellent aha moment. I am pretty confident that the root cause of the original bug is what I've identified here.

I agree that the mysterious off-by-three is too magical. I am hoping that this step toward a solution will prompt someone to point out what I missed.

@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Aug 24, 2025
@mqrause
Copy link
Contributor

mqrause commented Sep 2, 2025

From what I can tell, the calculation for the path doesn't take the final "go to center of omt" into account.

It's calculating this path, that stops at the corner of the target omt
path2

against this path, that stops in the middle of the edge of the target omt
path1

where the first one is somewhat obviously shorter. Both have one straight length of forest, but the first only has one straight length of field, wheras the second has half a straight and half a diagonal length of field.

This is what the actual calculation should result in, going to the actual center of the target omt, where the straight path would be preferable, because going diagonally through a field is cheaper than going diagonally through a forest.
path_should

Figuring out the need for the +1 felt like an excellent aha moment. I am pretty confident that the root cause of the original bug is what I've identified here.

In this example, adding +4 to avoid the zigzagging actually works, because the difference between the two paths is 7, and each diagonal adds 4, making the second path barely shorter. With longer paths with more diagonals, you can make any of the smaller numbers work as well, which should be a clear indication that it's not the correct solution.

@sparr sparr force-pushed the fix_pathfinder_zigzag branch from d6f7b3f to ecbc144 Compare September 3, 2025 01:44
@sparr sparr marked this pull request as draft September 3, 2025 01:44
@sparr
Copy link
Member Author

sparr commented Sep 3, 2025

Converting to draft while I attempt to address the problem of the last half-map of travel not being accounted for in the path cost analysis.

@sparr
Copy link
Member Author

sparr commented Sep 3, 2025

My current effort is based on the idea of adding a fake pathfinding node connected to the center direction of the destination, and not ending pathfinding until the optimal path to THAT node is found, instead of the current behavior that ends after finding the "optimal" path to [the edge of] the destination map.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Sep 3, 2025
@sparr sparr force-pushed the fix_pathfinder_zigzag branch from ecbc144 to 823c111 Compare September 3, 2025 11:48
@sparr
Copy link
Member Author

sparr commented Sep 3, 2025

I tried a few more approaches and decided that the increasing complexity isn't worth the better pathfinding in a few edge cases. I've updated this PR to undo the changes from #77556 that allowed pathfinding along map edges. This brings us back to pathfinding from OMT center to OMT center, but keeps diagonal pathfinding.

@sparr sparr changed the title Fix pathfinder zigzagging Fix pathfinder zigzagging by only pathing between OMT centers Sep 3, 2025
@sparr sparr marked this pull request as ready for review September 3, 2025 11:54
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Sep 3, 2025
@sparr sparr force-pushed the fix_pathfinder_zigzag branch from 823c111 to 83ebbe5 Compare September 7, 2025 14:50
@github-actions github-actions bot added the Code: Tests Measurement, self-control, statistics, balancing. label Sep 7, 2025
@sparr
Copy link
Member Author

sparr commented Sep 8, 2025

I'm not sure why the tests fail only on certain compilers/OSes. I'll be investigating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

astyled astyled PR, label is assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. json-styled JSON lint passed, label assigned by github actions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants