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

Extract AIUtils and HarvesterManager from HackyAI #14889

Merged
merged 4 commits into from Apr 6, 2018

Conversation

Projects
None yet
5 participants
@reaperrr
Copy link
Contributor

reaperrr commented Mar 8, 2018

As another step towards making HackyAI more modular (and making these modules accessible by other AI types), I've extracted a few utils and harvester management to separate files.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 8, 2018

Note: Right now the harv manager crashes, but I opened the PR anyway to hopefully get it debugged faster this way.

Edit: Nevermind, Travis already pointed out the issue(s).

@reaperrr reaperrr force-pushed the reaperrr:modular-AI1 branch from 38d489b to 86e8164 Mar 8, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 8, 2018

Updated.

claimLayer = world.WorldActor.TraitOrDefault<ResourceClaimLayer>();
}

CPos FindNextResource(Actor actor, Harvester harv)

This comment has been minimized.

@PeterAntal

PeterAntal Mar 9, 2018

Contributor

Devil's advocate/question from ignorance- Is there a reason FindNextResource method doesn't belong as a member of Harvester? (param overhead or containment of AI responsability might be)

I ask, as conceptually to me, it seems like this is operating in terms of next resource from pov of that harvester.

This comment has been minimized.

@reaperrr

reaperrr Mar 9, 2018

Author Contributor

I didn't write this part back when it was introduced, but iirc it's

containment of AI responsability


public void GiveOrdersToIdleHarvesters(List<Actor> activeUnits)
{
if (resLayer == null && resLayer.IsResourceLayerEmpty)

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 9, 2018

Member

This && wants to be an ||.

@@ -808,9 +747,7 @@ void FindNewUnits(Actor self)

foreach (var a in newUnits)
{
if (a.Info.HasTraitInfo<HarvesterInfo>())
QueueOrder(new Order("Harvest", a, false));

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 9, 2018

Member

This change isn't resembled in the new manager. Is it save to drop this order?

This comment has been minimized.

@reaperrr

reaperrr Mar 9, 2018

Author Contributor

It should be. I can't imagine any situation in which the HarvManager won't catch and give an order to an idle harvester.
And as long as there are resources in the vicinity of the production facility that spawned it, the harvesters' own FindResources will trigger.

So as far as I can tell, this Order was redundant.

This comment has been minimized.

@GraionDilach

GraionDilach Apr 6, 2018

Contributor

I've seen it happenning that since AI new units reach the rally point with an attack-move sometimes the AI used new war miners in base defence due to the attack move. But these units went to harvest after they defeated the threat in the base and started idling thenafter, so I presume this'll be fine as well.

@reaperrr reaperrr force-pushed the reaperrr:modular-AI1 branch from 86e8164 to 6e273cd Mar 18, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 18, 2018

Updated.

{
var cells = world.ActorsHavingTrait<T>().Where(a => a.Owner == player);

// TODO: Properly check building foundation rather than 3x3 area.

This comment has been minimized.

@pchote

pchote Mar 23, 2018

Member

This comment appears to be wrong?

This comment has been minimized.

@GraionDilach

GraionDilach Mar 23, 2018

Contributor

It isn't wrong. The tiles are enumerated in 3x3 blocks (since it checks a tile and all it's neighbours), ignoring the actual foundation.

@pchote
Copy link
Member

pchote left a comment

A couple more minor nits

@@ -656,8 +619,7 @@ void AssignRolesToIdleUnits(Actor self)
if (--assignRolesTicks <= 0)
{
assignRolesTicks = Info.AssignRolesInterval;
if (resLayer != null && !resLayer.IsResourceLayerEmpty)
GiveOrdersToIdleHarvesters();
harvManager.GiveOrdersToIdleHarvesters(activeUnits);

This comment has been minimized.

@pchote

pchote Mar 23, 2018

Member

Can we rename this to AssignRoles or Tick or something else a bit more generic?

if (a.Info.HasTraitInfo<HarvesterInfo>())
QueueOrder(new Order("Harvest", a, false));
else
if (!a.Info.HasTraitInfo<HarvesterInfo>())

This comment has been minimized.

@pchote

pchote Mar 23, 2018

Member

Can we add harvesters to Info.UnitsCommonNames.ExcludeFromSquads in the yaml and then drop this explicit check?

This comment has been minimized.

@GraionDilach

GraionDilach Apr 6, 2018

Contributor

@reaperrr consider a 👍 from me when this nit is resolved.

reaperrr added some commits Feb 23, 2018

Extract a HarvesterManager from HackyAI
This takes action when AI harvesters don't find ore near the base or became idle for some other reason.

@reaperrr reaperrr force-pushed the reaperrr:modular-AI1 branch from 6e273cd to de0ca63 Mar 24, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 24, 2018

Updated.

For the sake of completeness, I added a preliminary update rule, although it doesn't seem to work and I don't know why. Maybe someone can help debugging it.

@pchote

This comment has been minimized.

@reaperrr reaperrr force-pushed the reaperrr:modular-AI1 branch from de0ca63 to 7082d74 Mar 31, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 31, 2018

Updated. Tell me when I can remove the test case.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

Ohsorry, 👍 overlooked the last commit. ^^

@reaperrr reaperrr force-pushed the reaperrr:modular-AI1 branch from 7082d74 to 8790e51 Apr 6, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Apr 6, 2018

I consider @abcdefg30's +1 still valid, since all I adressed since his approval were pchote's nits and the update rule, so LGTM.

@reaperrr reaperrr merged commit 563c8ad into OpenRA:bleed Apr 6, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@reaperrr reaperrr deleted the reaperrr:modular-AI1 branch May 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.