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

Make harvester orders queueable #16193

Merged
merged 6 commits into from Apr 4, 2019

Conversation

@tovl
Copy link
Contributor

commented Feb 10, 2019

Fixes #14775
Fixes #11264
Fixes #16205
Fixes #15997

Harvester commands can now be queued before or after any other command without overwriting existing order queues. This PR is also part of the effort to get rid of all self-referential uses of SequenceActivities and replace them with an approach based on ChildActivities as discussed in #15632 and #16069.

In order to make Move work as a ChildActivity without glitches I had to change one line in Activity. This is because otherwise the childactivities of Move itself will not finish properly. This change doesn't seem to have any adverse consequences, but perhaps @obrakmann can comment on whether this is a good approach.

@pchote

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

In order to make Move work as a ChildActivity without glitches I had to change one line in Activity. This is because otherwise the childactivities of Move itself will not finish properly.

Aha, this will be why I couldn't get them to work in #16150.

@obrakmann

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2019

In order to make Move work as a ChildActivity without glitches I had to change one line in Activity.

I think this should be ok, and I can see now how it would cause bugs with Move.

@tovl tovl force-pushed the tovl:queue-harvester branch from 65c227d to 97000dd Feb 21, 2019

@chrisforbes
Copy link
Member

left a comment

This looks really good. I haven't tested thoroughly to ensure not breaking anything, but cleaning up the "do some other work and then queue myself again to take another look" model is great.

@pchote pchote referenced this pull request Feb 23, 2019
@pchote

This comment has been minimized.

Copy link
Member

commented Feb 24, 2019

When returning from unloading, the harvester now appears to wait for a short time on top of the ore patch before it starts harvesting.

@pchote

This comment has been minimized.

Copy link
Member

commented Feb 24, 2019

It would be great if this could also fix #16192, which also seems to be caused by self-referential uses of SequenceActivities.

@pchote pchote added this to the Next + 1 milestone Feb 24, 2019

@tovl tovl force-pushed the tovl:queue-harvester branch from 97000dd to cc2a710 Feb 24, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

When returning from unloading, the harvester now appears to wait for a short time on top of the ore patch before it starts harvesting.

I can not replicate this. However, if there is some pause it would probably be solved by using the pre-tick from #16217.

@tovl tovl referenced this pull request Feb 27, 2019

@tovl tovl force-pushed the tovl:queue-harvester branch from cc2a710 to 8b05653 Feb 28, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

Rebased onto #16246 and fixed micro-pauses by using the ChildActivity pre-tick option introduced in that PR.

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

When returning from unloading, the harvester now appears to wait for a short time on top of the ore patch before it starts harvesting.

I can not replicate this. However, if there is some pause it would probably be solved by using the pre-tick from #16217.

A simple and reliable repro case is to install the test map included in the zip file in #16192, build a refinery, and then watch the automated behaviour. After dropping the first full load, the harvester will return to the ore and wait for several seconds before continuing.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

#16192 is not primarily caused by self-referential uses of SequenceActivities, but rather by the complete mess that is UnblockRefinery. I have added a rework that properly fixes #16192, #16249 and #16205.

@tovl tovl force-pushed the tovl:queue-harvester branch from 6d24d47 to 7f84c23 Mar 2, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

Okay. I can finally reproduce the pause on that test map, but not on my latest commit. So I guess that was another bug caused by the flawed unblocking behavior, which is now fixed.

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

Great work! Beware #16253 which added a bodge to resolve the harvester issues for the pending release, which you'll need to revert again here if you haven't already.

The worst of the release work is hopefully done now, so I hope to be able to spend time reviewing your activity PRs in the next couple of weeks.

@tovl tovl force-pushed the tovl:queue-harvester branch 2 times, most recently from dd1ef8a to 8f4d0f5 Mar 3, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2019

Rebased.

@eskimo4130 The first and third issues should be fixed by this PR. The second issue is a separate problem and is something for a follow-up PR.

@pchote pchote removed the PR: Rebase me! label Mar 24, 2019

@pchote
Copy link
Member

left a comment

I can't overstate how much of an improvement this is over the current logic - really great job here.

I haven't tested the latest version ingame, but have gone through the code in detail and have a few new suggestions and questions that I think would improve things even further, plus some more general thoughts for things we should tackle in future PRs:

OpenRA.Mods.Common/Activities/DeliverResources.cs Outdated Show resolved Hide resolved
if (IsCanceling || isDocking)
return NextActivity;

if (targetActor != null && targetActor.IsInWorld)

This comment has been minimized.

Copy link
@pchote

pchote Mar 25, 2019

Member

If targetActor and harv.LinkedProc were Targets then we would get the null, alive, and in-world checks for free. It would also give us a check for ownership changes. I wonder whether it would be worthwhile to change this, but perhaps best left to a future PR.

This comment has been minimized.

Copy link
@pchote

pchote Mar 25, 2019

Member

Thinking about this further, is it valid from a gameplay perspective to fall back to the nearest best refinery if targetActor != null but !targetActor.IsInWorld (i.e. it died before we got here). I don't know the answer to this question, but if it is "no" then we should be cancelling the activity instead.

This comment has been minimized.

Copy link
@tovl

tovl Mar 26, 2019

Author Contributor

I think we should minimize the occurrences of harvesters idling without a prompt from the player. It's usually not a welcome surprise if your attention is elsewhere and you find out your harvesters were doing nothing.

So I think this is correct behaviour.

OpenRA.Mods.Common/Activities/DeliverResources.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/FindResources.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/HarvestResource.cs Outdated Show resolved Hide resolved
return ActivityUtils.SequenceActivities(self, mobile.MoveTo(closestHarvestablePosition.Value, 1), new HarvestResource(self), this);
}
self.SetTargetLine(Target.FromCell(self.World, closestHarvestablePosition.Value), Color.Red, false);
QueueChild(self, mobile.MoveTo(closestHarvestablePosition.Value, 1), true);

This comment has been minimized.

Copy link
@pchote

pchote Mar 25, 2019

Member

A good followup improvement for a future PR will be to move this and the MovingToResources notification above inside HarvestResource

This comment has been minimized.

Copy link
@tovl

tovl Mar 26, 2019

Author Contributor

Sounds reasonable. This refactor can be included when fixing #14752.

OpenRA.Mods.Common/Traits/Harvester.cs Outdated Show resolved Hide resolved

@tovl tovl force-pushed the tovl:queue-harvester branch 2 times, most recently from 9dab073 to 53e9ecd Mar 26, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

Now actually committed the changes.

OpenRA.Mods.Common/Activities/DeliverResources.cs Outdated Show resolved Hide resolved
// If no resources are found near the current field, search near the refinery instead.
// If that doesn't help, give up for now.
if (harv.LastHarvestedCell != null)
harv.LastHarvestedCell = null;

This comment has been minimized.

Copy link
@pchote

pchote Mar 27, 2019

Member

harv.LastHarvestedCell is already null, or we set it to null - so this could move outside the if.

This comment has been minimized.

Copy link
@tovl

tovl Mar 29, 2019

Author Contributor

Not really; See comment below.

if (harv.LastHarvestedCell != null)
harv.LastHarvestedCell = null;
else
harv.LastSearchFailed = true;

This comment has been minimized.

Copy link
@pchote

pchote Mar 27, 2019

Member

Do you know why this is set conditionally? Doesn't !closestHarvestablePosition.HasValue mean that the search failed, regardless of harv.LastHarvestedCell's value?

This comment has been minimized.

Copy link
@tovl

tovl Mar 29, 2019

Author Contributor

It is easier to see if you take into account that the logic is divided over two ticks here. Only after the second time, should the search be considered failed. It is probably not a good idea to spread it over two ticks though, so I've refactored this to only need one tick and make it hopefully clearer what happens here.

else
harv.LastSearchFailed = true;

var lastproc = harv.LastLinkedProc ?? harv.LinkedProc;

This comment has been minimized.

Copy link
@pchote

pchote Mar 27, 2019

Member

Can you explain why we prioritize the last linked proc over the currently(?) linked one? DeliverResources uses LinkedProc.

This comment has been minimized.

Copy link
@tovl

tovl Mar 29, 2019

Author Contributor

LastLinkedProc is set upon completing a delivery. so LastLinkedProc is the refinery the harvester is coming from, LinkedProc is the refinery the harvester is going towards. This block is making sure the harvester vacates the refinery it is coming from if it has nowhere else to go. The fallback would only trigger if the search failed immediately on building a new refinery.

dockOrder.Queue(self, new CallFunc(() => dockedHarv = harv, false));
dockOrder.Queue(self, DockSequence(harv, self));
dockOrder.Queue(self, new CallFunc(() => dockedHarv = null, false));
dockOrder.QueueChild(self, new CallFunc(() => dockedHarv = harv, false));

This comment has been minimized.

Copy link
@pchote

pchote Mar 27, 2019

Member

Another nice followup PR will be to rework IAcceptResources to expose a method to return the DockSequence activity plus notifications for start/end dock, and then this child queuing can move inside DeliverResources where it belongs.

Do you have a list of specific (harvester related) points you intend on working on after this PR is merged?
We should create an issue with these so we don't forget anything before the next release, and perhaps divide the tasks over a couple of people.

This comment has been minimized.

Copy link
@tovl

tovl Mar 29, 2019

Author Contributor

The only specific point right now that I intend to work on after this PR is a fix for harvesters getting stuck around blossom trees / ore pits. This seems to me the most pressing issue still remaining and I already have an idea for how to fix it. I could take on some minor refactors of HarvestResources while I'm at it.

Aside from that, I have some vague ideas for new features (refinery rally points, reintroducing refinery locking in a more user friendly way) and refactors, but nothing I want to commit myself to finishing (or even starting) before the next release.

This comment has been minimized.

Copy link
@pchote

pchote Mar 29, 2019

Member

Ok. I'll try on the weekend to collate my list of future-harv-pr-thoughts from above into a new issue that others can then add to.

@tovl tovl force-pushed the tovl:queue-harvester branch 2 times, most recently from 2eb3c3b to fee9004 Mar 29, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

Rebased.

@tovl tovl force-pushed the tovl:queue-harvester branch from fee9004 to c2c2860 Mar 30, 2019

@tovl tovl force-pushed the tovl:queue-harvester branch from c2c2860 to 71091c6 Mar 30, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

Rebased again.

@pchote
pchote approved these changes Mar 31, 2019

@pchote pchote added the PR: Needs +2 label Mar 31, 2019

@matjaeck
Copy link
Contributor

left a comment

Awesome work, couldn't find any gameplay regressions and being able to queue harvesting from specific cells will be a welcome new feature in the RA community I assume.

@reaperrr reaperrr merged commit ea4f24d into OpenRA:bleed Apr 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.