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

Refactor astar pt2 #3320

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

dkargin
Copy link
Contributor

@dkargin dkargin commented Jul 23, 2023

Making pathfinding contexts great reusable again.

@dkargin dkargin force-pushed the refactor_astar_pt2 branch 5 times, most recently from fecb13e to 75e5168 Compare July 31, 2023 18:55
@dkargin dkargin marked this pull request as ready for review August 6, 2023 18:52
@dkargin
Copy link
Contributor Author

dkargin commented Aug 6, 2023

All pathfinding requests are done in reverse now. It allows to effectively reuse previous searches for group orders.
Additionally: reworked debug rendering of pathfinding system

@dkargin dkargin marked this pull request as draft August 6, 2023 19:20
Copy link
Member

@past-due past-due left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of questions, from an initial quick look-over.

Looking forward to testing this!

src/fpath.cpp Outdated Show resolved Hide resolved
src/drawPath.cpp Outdated Show resolved Hide resolved
@dkargin dkargin marked this pull request as ready for review August 7, 2023 18:34
@KJeff01
Copy link
Member

KJeff01 commented Aug 15, 2023

I looked this over a few days ago and will give it some tests later this week.

@past-due
Copy link
Member

past-due commented Aug 19, 2023

@dkargin: I can reproduce a terrible lag, and have attached a save game (and map) that exhibits it:

lag-savegame-skirmish.zip

  • Copy the NTW-Arizona-v55-lag folder into <config dir>/savegames/skirmish/
  • Copy the 4c-NTW-Arizona-v55-a7b6e0aef64362ed647e469afc243e55ba0a4a83a48f38fb1a445c77220a4b5c.wz map into <config dir>/maps/

When loading this save game you should see lag that brings FPS down into single digits. (Culprit is the pathfinding thread.)

The cause is group 1 - the group of units in the middle-right. (They should all be selected when loading the save.)
If you order them away from the scavenger weapons on the right ridge (or press the "H" key on your keyboard, if the lag is making it near impossible to do anything), it will resolve.

If you select them all and order them to attack one of the scavengers weapons up on the right ridge, it will trigger it again (and continue until they are halted).

Of note: that ridge is obviously a different "continent" (in WZ terms), so the game should already know the units can't physically move onto that spot?

This is something new introduced in this PR, as testing the same save without the PR doesn't yield the pathological lag.

Additional findings:

  • In current master there is a temporary lag blip when issuing the order to attack something on that ridge, but it then resolves completely and group 1 just tries to pack as close to the ridge edge as possible.
  • If I back out the Astar pt1 PR altogether, this is no temporary lag blip when issuing the order to attack something on that ridge, and group 1 just tries to pack as close to the ridge edge as possible

So some edge case situation started in the pt1 PR, and is dramatically worse with the changes in this PR.

EDIT: The original culprit that introduced a (temporary) lag spike blip (from astar pt 1) is: a65e3f8

src/astar.cpp Outdated
// We have tried going to tileDest before.
if (pfContext.map[tileOrig.x + tileOrig.y * mapWidth].iteration == pfContext.iteration
&& pfContext.map[tileOrig.x + tileOrig.y * mapWidth].visited)
if (pfContext.isTileVisited(pt))
{
// Already know the path from orig to dest.
endCoord = tileOrig;
Copy link
Member

@past-due past-due Aug 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
endCoord = tileOrig;
endCoord = tileOrig;
break;

Probably missing a break here? (This seems to make it harder to trigger the endless lag case, but there must be another path because it's still possible.)

@past-due
Copy link
Member

past-due commented Aug 19, 2023

The following diff seems to eliminate the endless lag seems to reduce some of the ways to trigger it (and maybe makes the endless case not drive the FPS quite as low), but it's still possible to trigger lag if you have group 1 target one an emplacement AA laser on the ridge, and then order them to target one of the Emplacement-HvART-pit on the ridge.

NOTE: You must first halt all of group 1 by pressing "H" after loading the save.

diff --git a/src/astar.cpp b/src/astar.cpp
--- a/src/astar.cpp
+++ b/src/astar.cpp
@@ -479,7 +479,7 @@ ASR_RETVAL fpathAStarRoute(std::list<PathfindContext>& fpathContexts,
 
        ASR_RETVAL retval = ASR_OK;
 
-       bool mustReverse = false;
+       bool mustReverse = true;
 
        const PathCoord tileOrig = psJob->blockingMap->worldToMap(psJob->origX, psJob->origY);
        const PathCoord tileDest = psJob->blockingMap->worldToMap(psJob->destX, psJob->destY);
@@ -514,6 +514,8 @@ ASR_RETVAL fpathAStarRoute(std::list<PathfindContext>& fpathContexts,
                {
                        // Already know the path from orig to dest.
                        endCoord = tileOrig;
+                       mustReverse = false;
+                       break;
                }
                else if (pfContext.nodes.empty()) {
                        // Wave has already collapsed. Consequent attempt to search will exit immediately.
@@ -529,6 +531,7 @@ ASR_RETVAL fpathAStarRoute(std::list<PathfindContext>& fpathContexts,
                        ExplorationReport report = fpathAStarExplore(pfContext, pred, costLayer);
                        if (report) {
                                endCoord = pred.nearestCoord;
+                               mustReverse = false;
                                // Found the path! Don't search more contexts.
                                break;
                        }

Comment on lines -633 to -638

if (!context.isBlocked(tileOrig.x, tileOrig.y)) // If blocked, searching from tileDest to tileOrig wouldn't find the tileOrig tile.
{
// Next time, search starting from nearest reachable tile to the destination.
fpathInitContext(context, psJob->blockingMap, tileDest, pred.nearestCoord, tileOrig, dstIgnore);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be behind some of the issue?, given how the lag is produced and given the initial temporary lag spikes started in a65e3f8 which changed the value that could be passed in (pred.nearestCoord after that commit does not always equal context.nearestCoord before that commit)

1. Wrapped PF map cache into a standalone structure/instance
2. Moved all global state out of astar.cpp.
3. Using backward path search. It provides more opportunities for reusing
previous contexts.
4. Introduced cost accessor to A* wave exploration
Implemented debug rendering for pathginding:
 - renderer for blocking layer
 - rendering of current paths
 - drawing of path context
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants