Page MenuHomeWildfire Games

Petra: Fix behavior of guards when the guarded entity becomes garrisoned
ClosedPublic

Authored by Sandarac on Jun 15 2017, 8:39 AM.

Details

Summary

When a critical entity (i.e. hero in regicide) is given an order to garrison, any current guards it may have will stop following it when it garrisons in the holder. This means that the guards will stay positioned where they happened to be at the time (often in enemy territory) until the critical entity ungarrisons. This is especially common if the guarded is a fast cavalry hero that has just retreated from enemy territory.

moveToRange is used to keep the guards from "bunching up" against the garrison holder.

Test Plan

Guards will move to the guarded entity's garrison location when the guarded entity garrisons, instead of just stopping.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Sandarac created this revision.Jun 15 2017, 8:39 AM
Owners added a subscriber: Restricted Owners Package.Jun 15 2017, 8:39 AM
Vulcan added a subscriber: Vulcan.Jun 15 2017, 9:26 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1558/ for more details.

mimo added a subscriber: mimo.Jun 15 2017, 9:45 PM
mimo added inline comments.
binaries/data/mods/public/simulation/ai/petra/gameTypeManager.js
165 ↗(On Diff #2561)

As everything before is common to both cases, the test if (holderEnt.hasClass("Ship")) could be done only here? but not sure about it.

184 ↗(On Diff #2561)

I miss the point of the test on !Soldier. Shouldn't "if (plan = -2 || plan = -3)" always work?

Sandarac added inline comments.Jun 16 2017, 3:06 PM
binaries/data/mods/public/simulation/ai/petra/gameTypeManager.js
165 ↗(On Diff #2561)

I think it is okay to do two loops, as this one that tests on Ship will clear the hero guards, as well as update this.guardEnts, two things that don't need to be done to just send a move order to entities that are still guarding. Also, I don't think that the move order should be sent if the garrison holder is a ship, because of how ships have different access indices compared to land, so it could result in entities becoming stuck trying to move to the ship.

184 ↗(On Diff #2561)

Guards with the Soldier class are given "plan" metadata of -2 in tryAssignMilitaryGuard, so that they are not assigned to some attackPlan when they should be guarding. Healers aren't given this metadata when assigned to guard because they won't be tasked to anything else that Petra does. So if there is just the check for plan === -2 || plan === -3, military guards won't be given the move order.

mimo accepted this revision.Jun 17 2017, 10:31 AM
mimo added inline comments.
binaries/data/mods/public/simulation/ai/petra/gameTypeManager.js
165 ↗(On Diff #2561)

ok

184 ↗(On Diff #2561)

ok i'll accept it, but wouldn't then be better to also give plan=-2 for healers? just to avoid having to do such logic.

But also, all the stuff we currently do with "plan" is there only for historical reasons and seems to become unmanageable, so we should start to think to a better way to keep track of all the possible orders and their priorities for A23?

This revision is now accepted and ready to land.Jun 17 2017, 10:31 AM
This revision was automatically updated to reflect the committed changes.
Sandarac added inline comments.Jun 18 2017, 12:52 PM
binaries/data/mods/public/simulation/ai/petra/gameTypeManager.js
184 ↗(On Diff #2561)

But if plan=-2 is given to healers, healers will no longer retreat to heal themselves in a building when they are hurt (actually I'm not sure if it would be better for healers to stay guarding event if they have low health, or for them to retreat).

And yes, I agree that the system using the "plan" metadata should be improved, and especially I think that something different should be used for garrisoning.