Page MenuHomeWildfire Games
Differential D515

Petra: gameTypeManager cleanup
ClosedPublic
Actions

Authored by Sandarac on May 18 2017, 9:12 AM.

Details

Reviewers
mimo
Commits
rP19614: Petra: gameTypeManager cleanup
Summary

In regicide, data.garrisonEmergency is not reset when retreating a hero to a base when the hero couldn't find a place to garrison. It seems the best (only?) way to do this is to check for distance, but these calls could be expensive.

After this, there should be checks for "exposed" heroes to retreat to the second closest base if they can't garrison, as if a hurt hero is overwhelmed with enemies when it has already retreated it will not move anywhere. This will important for D104.

Test Plan

Read the code and/or check that the diff works as described.

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.May 18 2017, 9:12 AM
Vulcan added a subscriber: Vulcan.May 18 2017, 10:15 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!

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

Harbormaster completed remote builds in B1788: Diff 2011.May 18 2017, 10:16 AM
mimo added a subscriber: mimo.May 18 2017, 7:15 PM
mimo added inline comments.
binaries/data/mods/public/simulation/ai/petra/gameTypeManager.js
406 ↗(On Diff #2011)

Wouldn't it be simpler to reset here data.garrisonEmergency to false instead of doing it only when arriving (which forces you to test on the distance).

409 ↗(On Diff #2011)

I think there was a problem here: we should also update the stance value in the Map this.criticalEnts as it is used for comparison: maybe a better solution would be to pass the stance info to the AI (in AIProxy)

523 ↗(On Diff #2011)

same remark as before: data.stance should be updated

Sandarac added inline comments.May 19 2017, 8:51 AM
binaries/data/mods/public/simulation/ai/petra/gameTypeManager.js
406 ↗(On Diff #2011)

Okay.

409 ↗(On Diff #2011)

Yes, there was some discussion about adding getStance before, I guess it should be added now (for symmetry with setStance as well), although it seems this will involve adding a new messageType to UnitAI that will be sent whenever UnitAI.SetStance is called - MT_UnitStanceChanged. This will remove the need for the "stance" property in this.criticalEnts.

Sandarac updated this revision to Diff 2027.May 19 2017, 7:11 PM

Allow getStance() to be used by the AI.

mimo added a comment.May 19 2017, 7:35 PM

Patch looks good, i've just some minor comments inlined.
And having that easy access to the stance will allow to use it more inside petra in the future to improve the unit behaviour :)

binaries/data/mods/public/simulation/ai/petra/gameTypeManager.js
36 ↗(On Diff #2027)

could become
let defaultStance = hero.hasClass("Soldier") ? "aggressive" : "passive";
if (hero.getStance() !== defaultStance)

hero.setStance(defaultStance);

to avoid useless calls.

409 ↗(On Diff #2027)

could be replaced by if (criticalEnt.getStance() !== undefined && criticalEnt.getStance() !== "passive") ? not necessary to specify Relic here

417 ↗(On Diff #2027)

no more needed

Sandarac updated this revision to Diff 2031.May 19 2017, 8:01 PM

Check for getStance() on init, remove uneeded setMetadata, and properly get garrisonEmergency.

Sandarac marked 4 inline comments as done.May 19 2017, 8:04 PM
Sandarac added inline comments.
binaries/data/mods/public/simulation/ai/petra/gameTypeManager.js
409 ↗(On Diff #2027)

Actually this check for relic was for gameplay, in order to prevent relics from being "chased" across the map, and relics have UnitAI and stances, so I don't think there needs to be the check for undefined.

407 ↗(On Diff #2031)

The previous diff didn't access garrisonEmergency correctly.

mimo added inline comments.May 19 2017, 8:17 PM
binaries/data/mods/public/simulation/ai/petra/gameTypeManager.js
409 ↗(On Diff #2027)

ok if Relics have stance (i've not yet played with relics) we should keep this check, but we may still have in the future another type of critical entities without stance, so an additional check that getStance !== undefined is nonetheless needed?

or maybe we should add in the entity.js setStance function an early return if getStance is undefined?

Sandarac added inline comments.May 19 2017, 8:29 PM
binaries/data/mods/public/simulation/ai/petra/gameTypeManager.js
409 ↗(On Diff #2027)

In this function, I believe it can be assumed that the criticalEnt passed to it has a stance, as it is used for retreating a unit (i.e. not a wonder).

But an early return in setStance would be good.

if (this.getStance() === undefined)
	return undefined;
Sandarac updated this revision to Diff 2034.May 19 2017, 8:30 PM

Check for getStance() in setStance().

Vulcan added a comment.May 19 2017, 9:35 PM

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!

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

Harbormaster completed remote builds in B1805: Diff 2027.May 19 2017, 9:35 PM
mimo accepted this revision.May 19 2017, 10:24 PM
This revision is now accepted and ready to land.May 19 2017, 10:24 PM
Closed by commit rP19614: Petra: gameTypeManager cleanup (authored by mimo). · Explain WhyMay 19 2017, 10:28 PM
This revision was automatically updated to reflect the committed changes.
Vulcan added a comment.May 20 2017, 12:54 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!

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

Harbormaster completed remote builds in B1809: Diff 2031.May 20 2017, 12:54 AM
Vulcan added a comment.May 20 2017, 1:03 AM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1257/
See console output for more information: http://jw:8080/job/phabricator/1257/console

Harbormaster failed remote builds in B1810: Diff 2034!May 20 2017, 1:03 AM

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
simulation/
ai/
common-api/
3 lines
petra/
22 lines
components/
9 lines
3 lines
interfaces/
7 lines

Diff 2041

View Options

ps/trunk/binaries/data/mods/public/simulation/ai/common-api/entity.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/ai/petra/gameTypeManager.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/components/AIProxy.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/components/interfaces/UnitAI.js

Loading...
Wildfire Games · Phabricator