PathFinder status

[worktycho]

Let the mob pass a cWorld instance to the pathfinder via a special function that will be called whenever the mob's world changes (mob goes through a portal); this function should also reset all the pathfinding progress so far etc.

EDIT: Note that you can't pass the cWorld instance through the constructor, because at the time of construction the mob has no world assigned to it yet. The world is assigned in the cMonster::Initialize() function.

Or the pathfinder can take a cWorld in its constructor, and the mob can just not construct one until its initialize call. The mob can also construct a new one each time it switches world. That would simplify the pathfinder a lot.

Also can we switch DoWithChunk over to using a lambda function? It would be easier to use than the current functor classes system and there is only one other user.

[xoft]

I think the interface is designed rather poorly. The cPathFinder should have a cPath object in it that should wrap the path being calculated, it should return a const reference to that internal object in GetPath() (and GetPath should not modify it). Calling StartPathfinding (former findPath) should clear the path and start calculating, calling Step (former isCalculationFinished) will calculate a few steps of the path and return true if the calculation is finished. Calling Step after the calculation finishes is not an error; calling StartPathfinding before a path is found resets the current path and starts another search.

Your code style stinks. It may get through the CheckBasicStyle.lua script, but only because the script cannot check the style in all cases. We use spaces around ALL operators, we use CamelCase for function names, and exactly 5 empty lines between function bodies in cpp files. Preprocessor instructions should get regular indent. #includes should go first in the file, then everything else. And the preprocessor macro you use for sharing the source with another app is named just really terribly. Speaking of naming, names like "m_F", "m_G" etc. are useless.

[worktycho]

xoft you proposed API would have issues and I think the pathfinder will need to create a new path object each time. The problem is that if you have a zombie pathing to a player from a distance, then you want to be continually recaclulating the path. But you want to continue along the old path until the new one is finished, or else you get weird pauses. So the old path can not be deleted until the new one is ready. So using an internal path object is problematic.

---

Also if we transfer ownership to the client code it is simpler to manage. C++ clients can just use RAII, and lua can use the GC.

[xoft]

True, that could be an issue. Then how about having a SharedPtr to the path, initialized in StartPathfinding and returned in GetPath(). The path can then be shared between the pathfinder (for other clients calling GetPath()) and the actual path executor on the mob; possibly even exported to Lua as "planned path".

[worktycho]

But why bother with a shared pointer? The vast majority of paths are not going to be shared so the memory overhead of the shared_ptr reference count is likely to use more memory overall than just copying the path when needed. I'm coming round to the opinion that GetPath and IsCalculationFinished should be combined. Under the current API they can only be safely called together, so if they are part of the same method that enforces safely. Then the client is responsible for keeping hold of the path if they want to look at it again. cPathFinder is not thread safe so I'm no convinced we should expose the mob instances of the pathfinder to Lua. Instead cPathFinder should be exposed to lua through a dedicated wrapper which handles thread safety. Mobs can however safely expose the cPath object to lua because it is immutable. It is much easier to safely export a copy of the cPath to lua than the original object, which woudl then have complicated lifetime issues.

[xoft]

If it was just one function, there would be no way for it to signal an error, such as an unreachable destination. (exceptions are a big no-no in MCS!). other than that, I agree.

[worktycho]

The Path could be returned with a reference parameter, similar to how StringUtils::StringToInteger functions.

[LogicParrot]

I think the interface is designed rather poorly. The cPathFinder should have a cPath object in it that should wrap the path being calculated, it should return a const reference to that internal object in GetPath() (and GetPath should not modify it)

This is only good for the specific case of a mob interested in having 1 path at a time, e.g. a zombie chasing a player. But suppose some NPC would like to pre-calculate 10 paths only once.
Besides, this is exactly the "dumb, path spitting device" you requested.

Calling StartPathfinding (former findPath) should clear the path and start calculating, calling Step (former isCalculationFinished) will calculate a few steps of the path and return true if the calculation is finished. Calling Step after the calculation finishes is not an error; calling StartPathfinding before a path is found resets the current path and starts another search.

Valid points.
Error handling currently sucks, (And some of it is currently missing, like when the path is not reachable), it will be redesigned.

It may get through the CheckBasicStyle.lua script, but only because the script cannot check the style in all cases. We use spaces around ALL operators, we use CamelCase for function names, and exactly 5 empty lines between function bodies in cpp files. Preprocessor instructions should get regular indent. #includes should go first in the file, then everything else. And the preprocessor macro you use for sharing the source with another app is named just really terribly.

Ok, All valid criticism. will fix.
Except one point: __PATHFIND_DEBUG__ is precisely what the other app is, a Pathfinding debugger. Got a better name?

Speaking of naming, names like "m_F", "m_G" etc. are useless.

F,G,H is a standard A* naming scheme for the 3 values each cell has, where F = G + H, where H is the heuristics. any person with pathfinding experience knows what H,F,G are and I think it makes sense to name it that way.
http://theory.stanford.edu/~amitp/GamePr...rison.html
http://www.policyalmanac.org/games/aStarTutorial.htm

[LogicParrot]

@xoft: I'd like to use throws only when the interface is abused (E.g. calling Step before StartPathfinding or calling getPath before the calculation finishes.), they are supposed to crash the game and they shouldn't normally happen. Is this ok?

A crash is the easiest bug to detect.

[worktycho]

Generally the server uses the ASSERT macro, which is just a straight call to exit after logging the error. It has the advantage that most c++ run-times don't print then stacktrace on a unhanded exception and it makes it extremely clear that you are supposed to be able to recover from the error. It also isn't compiled into the release version as those sort of bugs aren't supposed to happen in the release version . But generally we agree with the fail fast idea.