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

Pathfinding #211

Closed
wants to merge 11 commits into from
Closed

Pathfinding #211

wants to merge 11 commits into from

Conversation

@Supermanu
Copy link
Contributor

@Supermanu Supermanu commented Dec 31, 2017

This PR shows the work in progress related to pathfinding and must not be included as is. I guess it still needs some clean up and refactoring but most of the work has been done. If you have any suggestions, comments or fixes, let me know!

What currently works:

  • A generic A* implementation that can tuned in each game. The output is a path of faces (and not a line to follow). It can be found in src/engines/aurora/astar.*
  • A Pathfinding class that handles the walkmesh where an A* instance can run and provides an interface to find paths.
  • The Pathfinding class also provides a smoothing algorithm that transforms a path of faces into a line that a player/creature can follow. It can also take the creature's size into account though it won't always give a nice path (it may go through unwalkable surface). Although this is only a temporary feature as explained below.
  • A KotOR implementation, that loads the walkmesh from a wok file.
  • A NWN implementation, that loads the walkmesh from a wok file. This implementation was trickier and some bugs may still come up. The difficulty was to connect the walkmesh of the different tiles in order to have one big walkmesh at the end. As I found no information related to tile connections, the current implementation tries to look if there is an adjacent walkable surface in the near neighborhood and makes some adjustment if needed.
  • A local walkmesh for dynamic content (only placeable up-to-now).
  • A graphical debug output that shows the result.
  • A showcase of what currently works in NWN in aa5507e. This is only for test and will be removed/refactored later.

TODO

Up to now, only the static content (everything that's in the wok file) is taken into account. All the dynamic contents as well as some static contents (like some crates) are not considered. The walkmesh for NWN and KotOR is made of triangles with different sizes and shapes which make it hard and not performance wise to modify on-the-fly. After some readings, I think that the best solution would be to create on-the-fly and frequently a small and local walkmesh made of squares (a grid) around the player/creature. It will use the output of the smoothing algorithm to know where it will have to go. It will directly include the creature's size into the walkmesh so it won't be necessary to compute it apart from A*.

That leads us to:

  • Build a grid from the static walkmesh,
  • Add dynamic content to it,
  • Take creature's size into account,
  • Make it fast,
  • Add tests for AABB,
  • Adapt to current walkmesh.
  • Clean up.
@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Jan 3, 2018

Very nice work already, thanks! Looking forward to see this progress. :)

About the build failures on our CIs, might be because your code uses features from a newer Boost version? Can you maybe try if there's a way you can downgrade your code to use features also available on the older Boost versions (1.54 on Travis CI, 1.59 on AppVeyor and xoreos' build docs say >= 1.53)?

If that isn't possible or feasible, we need to mandate a newer Boost version in our build docs. I'd prefer not to, but if it has to be, it has to be.

Updating Travis CI will be annoying, especially with the C++11 ABI change, so I will probably also upgrade the compilers; so the result might look like the Travis config in the Phaethon QT branch. Updating AppVeyor... I hope berenm has time to look at that, then. Looking at the AppVeyor config, I don't see where the Boost stuff is even coming from...

@Supermanu
Copy link
Contributor Author

@Supermanu Supermanu commented Jan 4, 2018

In deed, the code uses features from a newer Boost version though there are quite basic. As I'm not very satisfied with Boost.Geometry, I'm looking at CGAL right now which has everything that we need and should also work with "old" versions.

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Jan 4, 2018

Keep in mind that the version of CGAL in Ubuntu Trusty (which is what Travis CI runs) is 4.2. So you probably shouldn't use any of the newer features there either (whatever they may be), if that's possible. Since it's a C++ library, like Boost, it's probably also affected by the C++ ABI change, so pulling in a newer version from a more recent Ubuntu release could be problematic.

Unless CGAL is small enough that we might consider pulling it into xoreos, but from a cursory look at their Github page, it doesn't seem to be that way.

@Supermanu Supermanu force-pushed the Supermanu:pathfinding_wip branch 15 times, most recently from e1c1b2c to bca412f Jan 5, 2018
@Supermanu
Copy link
Contributor Author

@Supermanu Supermanu commented Jan 15, 2018

@mirv-sillyfish I'm looking right now at how to rasterize efficiently triangle mesh so I can build a grid and use it for pathfinding. I was wondering if there is a simple way to do that with the GPU i.e. send some triangles and get a boolean matrix of where triangles are in the XY plane? Otherwise, I'll stick with a software rasterizer.

@mirv-sillyfish
Copy link
Contributor

@mirv-sillyfish mirv-sillyfish commented Jan 15, 2018

The short version is "yes, it can be done". It's basically a texture read-back (use whatever terminology you wish - pbuffer, texture, whatever). Clear a framebuffer to a "blank" colour, render some appropriate geometry (simpler the better), and read it into ram. The resulting buffer is basically the averaged "height" of geometry (if any) in each cell.
Best approach might be to do this once per frame, but ping-pong between a couple framebuffers to prevent the GPU command pipeline stalling. Means the "current" matrix might be a frame out of date, but I suspect that won't really be an issue.
If logic and graphics are not coupled (as they shouldn't be), then there will need to be some kind of mechanism to stop one thread gleefully overwriting data currently in use, but again that's solvable.
Otherwise, yeah, definitely doable. Can encode a lot more data than boolean, and can also trim the clipping planes so that only geometry between a certain "height" band contributes.

@Supermanu
Copy link
Contributor Author

@Supermanu Supermanu commented Jan 16, 2018

Awesome! In deed, it doesn't need to be done instantly though it should be fast enough. I expect that it will send maximum 50-100 triangles and return a 128x128 matrix at the maximum; it will depend on how fast a path can be find inside such a grid and how good we want the path to be. I hope it doesn't require recent OpenGL though.

As I'm not familiar with OpenGL, I'll first implement a naive software rasterizer so I can check items on the TODO list. But then, I might need some help :P.

@Supermanu Supermanu force-pushed the Supermanu:pathfinding_wip branch from bca412f to fe016c8 Jun 3, 2018
@Supermanu Supermanu force-pushed the Supermanu:pathfinding_wip branch 3 times, most recently from 5cb90ac to aa5507e Jul 4, 2018
@Supermanu
Copy link
Contributor Author

@Supermanu Supermanu commented Jul 4, 2018

So I've made some progress. I've added the local grid walkmesh and it can take the creature's size and placeables into account though it misses doors and creatures. The built grid area is 65x65 with cell width about 0.1 which seems enough to me. The thing is, with larger grid the A* algorithm becomes quickly slower. To give an idea, finding the path, building the local walkmesh and finding the path on it takes, most of the time, less than 10 ms on my machine (AMD FX-8320). I feel that's a good start, and enough for this PR, but I think it could easily be improved by a factor 2 with the Jump point search optimization.
Finally, I still need to do some clean up, tests and adapt the walkmesh handling with the current code.

@Supermanu Supermanu force-pushed the Supermanu:pathfinding_wip branch from aa5507e to 5219014 Jul 4, 2018
@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Jul 4, 2018

Very nice, great seeing this progress :)

@Supermanu Supermanu force-pushed the Supermanu:pathfinding_wip branch 2 times, most recently from 72895e2 to c89bbdd Jul 4, 2018
@Supermanu
Copy link
Contributor Author

@Supermanu Supermanu commented Nov 9, 2018

I'm really sorry, I've been really busy the last two months.
Short answer, it's still not ready for nwn though it's better.
I rewrote some parts and it's now really better about tiles connections (I'm not sure I pushed the code). But I need to tackle one last tricky specific case. I came up with a solution that should work but hadn't correctly implemented.
Regarding kotor1/2, it has features parity with the current state but I guess it should need more testing just to be sure :P.

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Nov 10, 2018

Okay, nice to hear. Thanks for your continuing work :)

@Supermanu Supermanu force-pushed the Supermanu:pathfinding_wip branch 4 times, most recently from bead8bb to 01aa054 Jan 18, 2019
@Supermanu Supermanu force-pushed the Supermanu:pathfinding_wip branch 2 times, most recently from a8d5002 to f4d8478 Feb 2, 2019
@Supermanu
Copy link
Contributor Author

@Supermanu Supermanu commented Feb 2, 2019

At last, I think this PR is ready to test again. It could clearly be improved when it comes to performance and I guess it still could be tweaked to make the resulting path better.
One point though about NWN, some walkmesh files are, hmm, wrong (face adjacency are difficult to infer)? And finding a path through them gives weird paths. This is what the assert statement was all about. It is now better than before but still, in some very specific case, it fails. So I replaced the assert with a warning. I think it's not that much of an issue as the second step (the local pathfinding step) is independent from the face adjacency problem. And at the end, we still have a path.

An example of a weird walkmesh (the vertical face under the cursor is set as walkable):
walkmesh_weird_again

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Feb 2, 2019

Nice! :D

I'll stress-test and play around with it a bit!

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Feb 2, 2019

There's a crash (use after free) when leaving an area: https://gist.github.com/DrMcCoy/52a11e8dd20b9a38194435564d6464cf

@Supermanu Supermanu force-pushed the Supermanu:pathfinding_wip branch from f4d8478 to d2f2be3 Feb 3, 2019
Supermanu added 9 commits Jul 4, 2018
Move max, min and empty from private to protected.
This commit adds a generic A* algorithm as well as helper functions.
It also removes the current walkmesh implementation.
The y component in unproject() needed to be translated from the screen
coordinate to the OpenGL world screen coordinates. It was already done
in the getWorldObjectAt() function that uses unproject() internally but
prevented its use outside of getWorldObjectAt().
It now uses the pathfinding tools which takes profit of AABB and
creature's size (currently set to a constant).
It now uses the pathfinding tools which takes profit of AABB and
creature's size (currently set to a constant).
@Supermanu Supermanu force-pushed the Supermanu:pathfinding_wip branch from d2f2be3 to e9e78e9 Feb 3, 2019
@Supermanu
Copy link
Contributor Author

@Supermanu Supermanu commented Feb 4, 2019

Sorry, I forgot to run asan. It's now done and fixed :).

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Feb 4, 2019

Yeah, looks pretty well now. I couldn't find anything bad or crashy or weird, in fact. :D

What are your plans? What do you want me to do? Should I merge the code (minus e9e78e9, I guess)?

@Supermanu Supermanu force-pushed the Supermanu:pathfinding_wip branch from e9e78e9 to 87e56c9 Feb 5, 2019
@Supermanu
Copy link
Contributor Author

@Supermanu Supermanu commented Feb 5, 2019

Yes, you could merge the code minus 87e56c9. I think it's now a good basis to iteratively work on.
As for the future, there are two possibilities: performance and actual use of pathfinding (though it is partially used in kotor with this PR). If anyone wants to work on the latter, feel free to do so.

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Feb 5, 2019

Okay, merged as 53db5ed...685a464. Thanks! :D

@DrMcCoy DrMcCoy closed this Feb 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants