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

Restore transient blocker check in pathfinding queries. #17531

Merged
merged 2 commits into from Feb 12, 2020

Conversation

@pchote
Copy link
Member

pchote commented Dec 31, 2019

Fixes #17527.

@tovl: Can you comment on which of the issues closed by #16408 should be reopened with this?

@pchote pchote added this to the Next+1 milestone Dec 31, 2019
@tovl

This comment has been minimized.

Copy link
Contributor

tovl commented Dec 31, 2019

With the above comment taken into account we don't need to reopen any of those old issues. The backup search with BlockedByActor.None will make sure that units will still find some way if there is one. The main goal for #16408 was to make sure units always reach their destinations if at all possible, not necessarily to get there in the most efficient way possible.

However, the change here does mean that the initial search will fail more often on a combination of moving and stationary blockers. Since the backup search ignores all transient actors, we might be seeing a lot more wall hugging behaviour, which is inefficient and looks dumb.

In order to prevent that, it might be a good idea for now to have multiple tiers of backup pathing. So start with BlockedByActor.All; if that fails, search again with BlockedByActor.Immovable; if that search fails, do it again with BlockedByActor.Stationary, etc.

Context: The issue that this bit of code was fixing was #11437. But apparently I forgot to close it, so it doesn't need to be reopend either.

@pchote pchote force-pushed the pchote:fix-pathfinding-blockers branch from e2f7980 to cde207e Dec 31, 2019
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 31, 2019

Updated.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 31, 2019

Added a new commit to implement your second suggestion, but i'm not sure how to best test that.

@pchote pchote force-pushed the pchote:fix-pathfinding-blockers branch from f0285bc to 3b9cce7 Dec 31, 2019
@tovl

This comment has been minimized.

Copy link
Contributor

tovl commented Dec 31, 2019

i'm not sure how to best test that.

  • Build a wall around an enclosed area and leave one gap at a corner.
  • park a friendly vehicle in the gap (make sure there are no ways to slip past along diagonals).
  • Move another unit somewhere off to the side and then order it to go to the middle of the enclosed area.

With the first commit the unit cannot find a path initially and then ignores everything. The resulting behaviour should be that it drives straight into the wall and only then repaths.

with the second commit, after failing the first time, it will ignore only the friendly unit. The result should be that it immediately paths through the gap and nudges the friendly unit out of the way.

(This is all from theory; I haven't tested it myself yet)

@pchote pchote force-pushed the pchote:fix-pathfinding-blockers branch from 3b9cce7 to a28be21 Dec 31, 2019
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 31, 2019

Rebased to get us past the CI failures.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 31, 2019

with the second commit, after failing the first time, it will ignore only the friendly unit. The result should be that it immediately paths through the gap and nudges the friendly unit out of the way.

This does appear to solve part of the problem, but isn't sufficient by itself. The unit will now move next to the blocking friendly unit, but something else is preventing INotifyBlockingMove.OnNotifyBlockingMove from being called to nudge it out of the way.

@tovl

This comment has been minimized.

Copy link
Contributor

tovl commented Dec 31, 2019

There have been a number of PRs lately that touched upon the nudging code. It is possible one of them regressed this.

EDIT: Are you sure the unit wasn't within 8 cells of the destination? If so, then this is expected.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 31, 2019

I'd rather we fixed all the known issues in one place, will look into this as time allows.

@tovl

This comment has been minimized.

Copy link
Contributor

tovl commented Dec 31, 2019

Well if it is the nearEnough logic, then #16850 should provide a reasonable quick fix for that until we can implement TS style order distribution for ground units.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 1, 2020

Ok, it was indeed the nearEnough logic. I have filed #17541 to fix that.

@pchote pchote requested review from abcdefg30, reaperrr and teinarss Feb 12, 2020
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Feb 12, 2020

Leaving this open on the milestone is the worst case scenario - if we leave this until the last minute before a playtest we lose the opportunity for people to find bugs on bleed. Can we please get some activity here?

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Feb 12, 2020

I'm going to count @MustaphaTR's 👍 from #17541 as also valid here:

I'm not aware of any regression that may be caused by this in SP. This was implemented in SP to fix that while (a-)moving units didn't spread properly and create a straigt line.

@pchote pchote merged commit 3a688e0 into OpenRA:bleed Feb 12, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pchote pchote deleted the pchote:fix-pathfinding-blockers branch Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.