HomeWildfire Games

Decouple long and hierarchical pathfinders to an extent.

Description

Decouple long and hierarchical pathfinders to an extent.

Following rP22253, this decouples the hierarchical pathfinder and the long pathfinder. The long pathfinder was the class owning the hierarchical pathfinder, which didn't particularly make sense and resulted in some interface awkwardness.

At the moment, the long pathfinder still needs to hierarchical pathfinder to compute paths (to make sure they are reachable).

Differential Revision: https://code.wildfiregames.com/D1867

Event Timeline

elexis added a subscriber: elexis.Mon, Jun 24, 2:38 PM

(I didn't check the includes)

Points of content:

  • Moving logic from the/a pathfinder to the caller of the pathfinder. If you want to remove the logic of pathfinder1 in pathfinder2, move it to some meta-pathfinder that controls both.
  • Pointers vs. non-pointers.
  • A ranged loop.

I don't raise a concern because I am not aware of a hard defect in the code, but this shall not bias that I won't raise one in the future as

  • the claimed purpose in the commit message is in doubt
  • the review / alternative explanation stage should have mentioned why moving logic from the pathfinder to the caller of the pathfinder is a cleanup and not a cleanliness regression.

And again, please write commit messages carefully and avoid phrases such as:

  • didn't particularly make sense and resulted in some interface awkwardness. - the commit message shall inform, "doesnt make sense" and "awkwardness" is not an information
  • Decouple long and hierarchical pathfinders to an extent - what is the extent? (I don't ask because I want to know the answer, but the commit messages shall be written in such a way that it's clear what is meant)
/ps/trunk/source/simulation2/components/CCmpAIManager.cpp
506

IMO this is a regression in cleanliness (and the purpose of the diff was to clean), because now every pathfinder user (CCmpAIManager.cpp, CCmpPathfinder.cpp) of the one function has to reproduce the pathfinder logic, it is easy to introduce code-path differences (bugs).
There already is a lot of that duplication going on, so it should be reduced, not furthered IMO.

546

(same logic-duplication meh)

814

(same logic duplication argument)

934

(same logic duplication argument)

/ps/trunk/source/simulation2/components/CCmpPathfinder.cpp
61

Those wouldn't have had to be pointers AFAICS, nor m_VertexPathfinder from rP22253

217

If one has the choice between pointer and non-pointer variable, the latter is the one that can't null-deref, so it would be safer to not use pointers.

710

ack

/ps/trunk/source/simulation2/components/CCmpPathfinder_Common.h
43

good (and correct because the classes are only mentioned once without member access), making things faster to compile (even if negligible her).

172

ack

/ps/trunk/source/simulation2/helpers/HierarchicalPathfinder.cpp
358

Good place for insertion, since it's immediately below the related code.

Should have used a ranged loop I guess.

/ps/trunk/source/simulation2/helpers/LongPathfinder.cpp
1002

(Agree with the implementation move from .h to .cpp)

1007

(This adds the requirement that GenerateSpecialMap creates a m_Grid. I guess that passes, although an additional protection isn't so bad if the performance cost is negligible (here it is due to the expensive GenerateSpecialMap call.) One has the choice between using both functions at times or always using the one function, and adding the check to that function, merging the two.
But that's only reasonable if the !bool cost is negligible (it might not).)

/ps/trunk/source/simulation2/helpers/LongPathfinder.h
1

9

194

(same logic-duplication argument)

205

This call would be better kept in this place I think, because previously the code ensured that m_PathfinderHier.Update is always called when LongPathfinder::Update() was called, whereas now one has to ensure that in every place that calls it., which is one line of code insertion away from introducing code path differences, i.e. bugs.

212

I agree that this function should not be in this file, but the one it was moved to (HierarchicalPathfinder.h.)

wraitii added a comment.EditedMon, Jun 24, 4:48 PM

There is a simple, infaillible logic to making the hierarchical pathfinder not a sub-class of the long-range pathfinder: you can perfectly use the hierarchical pathfinder without a long-range pathfinder (the reverse isn't true).
If anything, we could make the hierarchical pathfinder own the long-range pathfinder, but the opposite didn't make sense.

I agree that it introduces some regrettable duplication of code, in the AI particularly, but I don't think it introduces too much danger, as these functions are always updated right next to one another.
Furthermore, I question why the long-range pathfinder keeps a grid itself since it doesn't particularly belong there, so these calls might be removed. The AI, also, might not need its own long-range pathfinder or hierarchical pathfinder if it allowed async calls.

Finally, I would suggest deleting this entirely, but that's a separate concern.


There us another argument for making the hierarchical pathfinder a member of the pathfinder: to expose functions form the hierarchical pathfinder, one used to have to write:

  • ICmpPathfinder interface
  • CCmpPathfinder implementation
  • LongPathfinder call forwarding
  • Hierarchical Pathfinder definition/implementation

Making the hierarchical pathfinder independent skips this middle and results in cleaner code there.


Finally:

Moving logic from the/a pathfinder to the caller of the pathfinder. If you want to remove the logic of pathfinder1 in pathfinder2, move it to some meta-pathfinder that controls both.

I would say this already exists in CCmpPathfinder.

/ps/trunk/source/simulation2/components/CCmpPathfinder.cpp
61

The vertex pathfinder is a pointer because I can't initialise a reference in the constructor (only place where it can be initialised). Having a separate "Init" function would work.

I then chose pointers here for consistency. The pathfinder helpers aren't necessarily part of the Pathfinder component - I guess we could rule that references are always preferable if they can be used, but so far we have no such rule.

/ps/trunk/source/simulation2/helpers/HierarchicalPathfinder.cpp
358

I think this was merely moving code, but fair enough