Differential D80
Long range pathfinder cleanup/speedup Authored by wraitii on Jan 21 2017, 10:46 AM.
Details
I was looking at the long-range pathfinder, hoping to see where the inefficiencies lied. I've posted my thoughts on the forum, but there are at least two things I believe we should do ASAP, and this diff does:
Validate that pathfinding still works I guess? This is relatively safe.
Diff Detail
Event TimelineComment Actions Build is green Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK! http://jw:8080/job/phabricator/217/ for more details. Comment Actions Build is green Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK! http://jw:8080/job/phabricator/218/ for more details. Comment Actions That looks nice but I'm just suggesting a small change so your new ENSURE is ensured by design. Didn't test yet.
Comment Actions I actually didn't change the state having the goal because we rely on calling PathGoal functions in a bunch of places and changing that would make it far uglier at the moment and I'm feeling too lazy to fix it properly for now. It should be done when we switch to the JPS cache, which ought to be done someday. Comment Actions Build is green Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK! http://jw:8080/job/phabricator/254/ for more details. Comment Actions The code looked good but when actually testing the patch, units now have a stupid behavior. Easiest to reproduce is giving a gathering order: the unit won't go there in a straight line but will follow a strange path. It must come from the OnTheWay changes. Comment Actions Mh, that is rather odd, must have messed something up. Will look into it whenever I start working on patches again. Comment Actions Fixes the above issue, there was some weird goal thing in the main function that made it go wrong. Comment Actions Successful build - Chance fights ever on the side of the prudent. Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Checking XML files... Comment Actions Looks good. Checked a replay for the fun of it and the hashes were identical. I'd add spaces around all the *, +, -, but I guess that's personal preference. In HierarchicalPathfinder.h, maybe we could be more explicit with "Updates @p goal to a point goal that's guaranteed...". /** * Updates @p goal so that it's guaranteed to be reachable from the navcell * @p i0, @p j0 (which is assumed to be on a passable navcell). * * If the goal is not reachable, it is replaced with a point goal nearest to * the goal center. * * In the case of a non-point reachable goal, it is replaced with a point goal * at the reachable navcell of the goal which is nearest to the starting navcell. */ void MakeGoalReachable(u16 i0, u16 j0, PathGoal& goal, pass_class_t passClass);
Comment Actions Just smalls conventions nitpicks for when you commit : https://trac.wildfiregames.com/wiki/Coding_Conventions I think comments should start with caps (484,492), and the conventions above states "As wrapped block comments are extremely unreadable. Always try to keep each line of those below 80 chars (counting tabs as 4 chars)." Comment Actions (In case of nitpicking I also wouldn't be opposed to having comments on separate lines, spaces between operators and x && y for x ? y : false where possible) Comment Actions
Done in D53 where I feel it fits better. Comment Actions Successful build - Chance fights ever on the side of the prudent. Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Checking XML files... Comment Actions Thanks for the review temple. I've checked again that hashes are similar on a 1v1 AI game on a normal map, confirmed correct. Running profiling seemed to show this was very, very slightly faster (by a few µs per call) which is as expected. Committing now. | ||||||||||||||||||||||||||||||||||||||||||||