Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AStarGrid pathfinding (and unit testing) #272

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

Spartan322
Copy link
Member

@Spartan322 Spartan322 commented Jan 24, 2025

Add unit testing
Add distance_squared function to vectors
Add dot function to vectors
Add hash_murmur3 function to Utility.hpp

@Spartan322 Spartan322 added the enhancement New feature or request label Jan 24, 2025
@Spartan322 Spartan322 force-pushed the add/astar branch 2 times, most recently from 76c6cb6 to 8492b6d Compare January 25, 2025 19:41
@Spartan322 Spartan322 changed the title Add AStarGrid pathfinding Add AStarGrid pathfinding (and unit testing) Jan 25, 2025
SConstruct Show resolved Hide resolved
@Spartan322 Spartan322 force-pushed the add/astar branch 3 times, most recently from 006da39 to ecb68dd Compare January 26, 2025 20:59
@schombert
Copy link

Which connections are possible to traverse varies depending on which nation you are considering (in part because of military access and the nations they are at war with) and is constantly changing from day to day (both due to the factors mentioned before changing and because access is dependent on province control, which may change frequently). Thus, this class seems fundamentally unfit for purpose because you would be forced to reconstruct the graph every time you want to perform a new path-finding check, which you will be doing constantly for the AI. Also, the std::deque implementation in msvc is terrible and little better than a linked list.

@Spartan322
Copy link
Member Author

This is a draft PR, there is no reason to review on the premise that its finished especially when its not passing the unit tests yet.

Thus, this class seems fundamentally unfit for purpose because you would be forced to reconstruct the graph every time you want to perform a new path-finding check

This class doesn't require reconstruction of the graph for pathfinding checks, the segments and points don't change unless the map itself changes and this class permits disabling points as needed, there are ways I could optimize that but that's not the point of this draft PR right now anyway, this class already lets you manually toggle the points without removing them, (which is a lot less expensive then reconstructing the graph) and it would be trivial to introduce heuristic and cost changes per nation with this class, also its an astar pathfinding algorithm. Worst case scenario if I didn't optimize it, each nation would have its own astar grid.

Also, the std::deque implementation in msvc is terrible and little better than a linked list.

That's hardly a concern for this case, if it truly is an issue, its trivial to switch out, even then that's only an issue with deques if you're constantly allocating to them in a way that causes fragmentation between the fixed arrays which is not intended to be done with the grid anyway and if truly mattered we can deal with that separately, the arrays inside of deques that are close together are still cache local. I don't care about performance concerns that aren't proven to be an issue for our use cases.

@schombert
Copy link

schombert commented Jan 27, 2025

Try to imagine using this to pathfind from A to B for a unit. Ok, so you know the nation that the unit belongs to. So, you need to determine which connections the unit can traverse before running solve. But, having access in victoria 2 is something that you have on a per province basis or not. So you need to iterate over all the connections in your graph and see whether the unit has access to the provinces on both ends to either enable or disable the connection. And then, after looking at every connection in the graph to turn it on or off, you can run solve. But you didn't need to do the vast majority of that work. Pathfinding will probably succeed or fail before traversing most of those connections, so you didn't need to iterate over most of them to determine whether they were potentially traversable in the first place. It is much faster to start at the province the unit is in, look at the adjacent provinces to filter out ones that the unit doesn't have access to, add those to the list of provinces to visit to pathfind, and then repeat until completion. If A is next to B, for example, you end up evaluating access for just the provinces adjacent to A, and then you are done, rather than doing that for the entire map.

Also, before you commit to trying to amortize this by computing it once per nation, you should also keep in mind that units are allowed to pathfind through at-sea transports that are empty if the units could fit within those transports. Thus, pathfinding must be done on a per-unit basis.

I posted my comment because the approach is fundamentally flawed in a way that will need revising regardless of whether it passes the tests.

@Spartan322 Spartan322 force-pushed the add/astar branch 7 times, most recently from 62fe7a1 to 776b0c4 Compare January 29, 2025 02:27
Add unit testing
Add `distance_squared` function to vectors
Add `dot` function to vectors
Add `hash_murmur3` function to Utility.hpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants