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

Fix Harvester to Refinery pathfinding #20183

Merged
merged 2 commits into from Aug 13, 2022

Conversation

RoosterDragon
Copy link
Member

Two changes here that fix the issues preventing harvesters from using multiple refineries. Fixes #20162


Commit 1:
Fix HierarchicalPathFinder failing to consider multiple source locations.

When asked to find a path from multiple source locations, the abstract search is used to determine which source locations are viable. Source locations that cannot be reached on the abstract graph are excluded from the local path search. As we know the locations are unreachable, this prevents the local path search from expanding over the entire search space in an attempt to find these unreachable locations, preventing wasted effort.

In order to determine these reachable locations, the abstract search is expanded successively trying to reach each source location each time. However, this failed to account for a property of the ExpandToTarget for which a comment is now added. If the location was found previously, then expanding to try and find it again will fail. If the source locations were close together, it was likely that the initial expansions of the search space would have included them, and thus they would not be found on a later expansion. This would mean these locations would incorrectly be thought unreachable.

To fix this, we check if the location has already been explored (has CellStatus.Closed in the graph). If so we can check the cost to determine if it is reachable.

Commit 2:
Allow custom cost to exclude source locations in path searches.

During the refactoring to introduce HierarchicalPathFinder, custom costs were no longer applied to source locations. The logic being that if we are already in the source location, then there should be no cost added to it to get there - we are already there!

Path searches support the ability to go from multiple sources to a single target, but the reverse is not supported. Some callers that require a search from a single source to one of multiple targets perform their search in reverse, swapping the source and targets so they can run the search, then reversing the path they are given so things are the correct way around again. For callers that also use a custom cost like the harvester code that do this in order to find free refineries, they might want the custom cost to be applied to the source location. The harvester code uses this to filter out overloaded refineries. It wants to search from a harvester to multiple refineries, and thus reverses this in order to perform the path search. Without the custom cost being applied to the "source" locations, this filtering logic never gets applied.

To fix this, we now apply the custom cost to source locations. If the custom cost provides an invalid path, then the source location is excluded entirely. Although this seems unintuitive on its own, this allows searches done "in reverse" to work again.

@darkademic
Copy link
Contributor

Confirmed that it fixes #20162 : Placed several refineries around the map, each harvester returned to their corresponding refinery, then on the ore drying up they went to the next nearest field and continued to return to the closest refinery. On manually selecting a refinery to return to, they did so, then went to harvest the closest ore.

@PunkPun
Copy link
Member

PunkPun commented Aug 11, 2022

For future it might be good to add a method for finding the closest CPos. Harvester doesn't need to find the path to all refineries, just to the closest one

@PunkPun
Copy link
Member

PunkPun commented Aug 11, 2022

I imagine this has a significant performance impact in lategame, especially with more players and large maps

@RoosterDragon
Copy link
Member Author

For future it might be good to add a method for finding the closest CPos. Harvester doesn't need to find the path to all refineries, just to the closest one

That is already how FindPathToTargetCell works, you'll get the shortest path back for one of the sources to the target. It doesn't have to find a path from each source to the target to do that. If you have multiple source locations then calling the method once with all the locations can be much faster than calling it multiple times with one location at a time.

PunkPun
PunkPun previously approved these changes Aug 11, 2022
chrisforbes
chrisforbes previously approved these changes Aug 11, 2022
…ons.

When asked to find a path from multiple source locations, the abstract search is used to determine which source locations are viable. Source locations that cannot be reached on the abstract graph are excluded from the local path search. As we know the locations are unreachable, this prevents the local path search from expanding over the entire search space in an attempt to find these unreachable locations, preventing wasted effort.

In order to determine these reachable locations, the abstract search is expanded successively trying to reach each source location each time. However, this failed to account for a property of the ExpandToTarget for which a comment is now added. If the location was found previously, then expanding to try and find it again will fail. If the source locations were close together, it was likely that the initial expansions of the search space would have included them, and thus they would not be found on a later expansion. This would mean these locations would incorrectly be thought unreachable.

To fix this, we check if the location has already been explored (has CellStatus.Closed in the graph). If so we can check the cost to determine if it is reachable.
During the refactoring to introduce HierarchicalPathFinder, custom costs were no longer applied to source locations. The logic being that if we are already in the source location, then there should be no cost added to it to get there - we are already there!

Path searches support the ability to go from multiple sources to a single target, but the reverse is not supported. Some callers that require a search from a single source to one of multiple targets perform their search in reverse, swapping the source and targets so they can run the search, then reversing the path they are given so things are the correct way around again. For callers that also use a custom cost like the harvester code that do this in order to find free refineries, they might want the custom cost to be applied to the source location. The harvester code uses this to filter out overloaded refineries. It wants to search from a harvester to multiple refineries, and thus reverses this in order to perform the path search. Without the custom cost being applied to the "source" locations, this filtering logic never gets applied.

To fix this, we now apply the custom cost to source locations. If the custom cost provides an invalid path, then the source location is excluded entirely. Although this seems unintuitive on its own, this allows searches done "in reverse" to work again.
@PunkPun PunkPun merged commit 2599cb2 into OpenRA:bleed Aug 13, 2022
@RoosterDragon RoosterDragon deleted the path-multi-source-fixes branch August 13, 2022 09:01
@PunkPun
Copy link
Member

PunkPun commented Aug 13, 2022

Changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Harvesters reroutes to Refinery are broken on bleed
6 participants