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
Prevent harvesters getting stuck in gridlock by allowing units to give way. #16408
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial thoughts:
This makes a fundamental change to our hottest code paths, so we really need to do some performance comparisons to make sure we don't unreasonably increase the overheads.
The second commit is a really big deal and is going to need some careful testing and analysis, so it might be a good idea to split the first commit into its own PR that we can merge quickly to resolve #16412.
|
||
if (cachedPath != null) | ||
return cachedPath; | ||
// Only cache path when transient actors are ignored, otherwise there is no guarantee that the path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any idea about how useful this cache actually is? Ping @teinarss.
- A lot has changed since this was added. If it doesn't do much in practice anymore then we should remove it completely to help simplify the pathfinder mess.
- If we have lots of repeated queries in the same tick then we should cache everything, but expire the non-
BlockedByActor.None
paths at the end of the tick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im guessing it still is valid for preventing spam click => lag spike.
64739d0
to
11baef7
Compare
I'm excited by the prospect of this superseding But on closer inspection it seems that the different |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried my hardest to get harvesters stuck and failed. GJ
#16328 should hopefully be merged this weekend, so it would be good to get this one rebased and for profiling. |
Needs a rebase, which will also pull in the benchmarking changes to help with profiling. |
Rebased. |
What is on the axes? |
X axis is ms and y axis is frequency.
…On Sun, Jun 9, 2019, 22:03 tovl ***@***.***> wrote:
What is on the axes?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16408?email_source=notifications&email_token=AESI7GA2JKHLSWARPLKH6JDPZVO2FA5CNFSM4HFYX3I2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXIRDWA#issuecomment-500240856>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AESI7GBJ7F3YC23Z6KFCR5TPZVO2FANCNFSM4HFYX3IQ>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The benchmark doesn't show hard perf regressions, so this should be good to go? (Regarding the unresolved review comments above.)
91422a9
to
71b967c
Compare
Rebased. |
71b967c
to
5e137a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks effective and well put together - some minor asks for you.
5e137a4
to
9434d39
Compare
This fixes the issues with harvesters getting stuck around refineries and resource spawns as well as the majority of other gridlocks caused by units moving in opposite directions where no alternative paths are available.
Fixes #14752
Fixes #12817
Fixes #12576
Fixes #10565
Fixes #3089
Fixes #13260
Fixes #16412
Fixes #3671
The first commit in this PR contains a general refactor ofHarvestResources
such as was discussed in Make harvester orders queueable #16193.